{"thread":{"id":"54005","subject":"\"#define precompose_argv(c,v) /* empty */\" is evil","startedAt":"2020-08-06T23:47:40Z","lastAt":"2020-08-07T07:56:57Z","messageCount":10,"participants":["Junio C Hamano","brian m. carlson","Jeff King","Torsten Bögershausen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"403104","messageId":"xmqqy2mribft.fsf@gitster.c.googlers.com","threadId":"54005","inReplyTo":null,"subject":"\"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-06T23:47:34Z","receivedAt":"2020-08-06T23:47:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I had seen an interesting compilation breakage today.  A topic adds\nmany more uses of argv-array API so I resolved semantic conflict\npatch until \"make builtins/submodule-helper.o\" passed.  I failed to\nspot one remaining breakage until I saw\n\n    https://travis-ci.org/github/git/git/jobs/715668996\n\nThe problematic line was\n\n    precompose_argv(diff_args.argc, diff_args.argv);\n\nwhere diff_args used to be an argv_array and is now a strvec.\n\nWhy didn't I catch this in my local test?\n\n$ git grep -n -e precompose_argv -- \\*.h\ncompat/precompose_utf8.h:31:void precompose_argv(int argc, const char **argv);\ngit-compat-util.h:256:#define precompose_argv(c,v)\n\nThe problematic part is this one used on all platforms other than macOS:\n\n    /* used on Mac OS X */\n    #ifdef PRECOMPOSE_UNICODE\n    #include \"compat/precompose_utf8.h\"\n    #else\n    #define precompose_str(in,i_nfd2nfc)\n    #define precompose_argv(c,v)\n    #define probe_utf8_pathname_composition()\n    #endif\n\nI am wondering if it is a good idea to use something like\n\n    static inline void precompose_argv(int argc, const char **argv)\n    {\n\t; /* nothing */\n    }\n\ninstead.  As long as the compiler is reasonable enough, this should\nnot result in any code change in the result, except that it would\nstill catch wrong arguments, even if these two parameters are unused\nand optimized out.\n\n"},{"id":"403107","messageId":"20200807000126.GC8085@camp.crustytoothpaste.net","threadId":"54005","inReplyTo":"xmqqy2mribft.fsf@gitster.c.googlers.com","subject":"Re: \"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-08-07T00:01:26Z","receivedAt":"2020-08-07T00:01:38Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-08-06 at 23:47:34, Junio C Hamano wrote:\n> I am wondering if it is a good idea to use something like\n> \n>     static inline void precompose_argv(int argc, const char **argv)\n>     {\n> \t; /* nothing */\n>     }\n> \n> instead.  As long as the compiler is reasonable enough, this should\n> not result in any code change in the result, except that it would\n> still catch wrong arguments, even if these two parameters are unused\n> and optimized out.\n\nYes, this seems like a prudent approach.  I believe it's widely used by\nthe Linux kernel, so presumably compilers are capable enough to optimize\nit out.  As you noted, it provides type checking for all platforms,\nwhich is nice.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"403108","messageId":"xmqqpn83i9sk.fsf@gitster.c.googlers.com","threadId":"54005","inReplyTo":"20200807000126.GC8085@camp.crustytoothpaste.net","subject":"Re: \"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T00:23:07Z","receivedAt":"2020-08-07T00:23:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2020-08-06 at 23:47:34, Junio C Hamano wrote:\n>> I am wondering if it is a good idea to use something like\n>> \n>>     static inline void precompose_argv(int argc, const char **argv)\n>>     {\n>> \t; /* nothing */\n>>     }\n>> \n>> instead.  As long as the compiler is reasonable enough, this should\n>> not result in any code change in the result, except that it would\n>> still catch wrong arguments, even if these two parameters are unused\n>> and optimized out.\n>\n> Yes, this seems like a prudent approach.  I believe it's widely used by\n> the Linux kernel, so presumably compilers are capable enough to optimize\n> it out.  As you noted, it provides type checking for all platforms,\n> which is nice.\n\nSo I hope the following (untested and not signed off yet) may lead\nus in the right direction?\n\n-- >8 --\nSubject: compat-util: type-check parameters of mocked functions\n\nWhen there is no need to run a specific function on certain platforms,\nwe often #define an empty function to swallow its parameters and\nmake it into a no-op, e.g.\n\n    #define precompose_argv(c,v) /* no-op */\n\nWhile this guarantees that no unneeded code is generated, it also\ndiscards type and other checks on these parameters, e.g. a new code\nwritten with the argv-array API (diff_args is of type \"struct\nargv_array\" that has .argc and .argv members):\n\n    precompose_argv(diff_args.argc, diff_args.argv);\n\nmust be updated to use \"struct strvec diff_args\" with .nr and .v\nmembers, like so:\n\n    precompose_argv(diff_args.nr, diff_args.v);\n\nafter the argv-array API has been updated to the strvec API.\nHowever, the \"no oop\" C preprocessor macro is too aggressive to\ndiscard what is unused, and did not catch such a call that was left\nunconverted.\n\nUsing a \"static inline\" function whose body is a no-op should still\nresult in the same binary with decent compilers yet catch such a\nreference to a missing field or passing a value of a wrong type.\n\nWhile at it, I notice that precompute_str() has never been used\nanywhere in the code, since it was introduced at 76759c7d (git on\nMac OS and precomposed unicode, 2012-07-08).  Instead of turning it\ninto a static inline, just remove it.\n\n---\n git-compat-util.h | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 5637114b8d..7a0fb7a045 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -252,8 +252,10 @@ typedef unsigned long uintptr_t;\n #ifdef PRECOMPOSE_UNICODE\n #include \"compat/precompose_utf8.h\"\n #else\n-#define precompose_str(in,i_nfd2nfc)\n-#define precompose_argv(c,v)\n+static inline void precompose_argv(int argc, const char **argv)\n+{\n+\t; /* nothing */\n+}\n #define probe_utf8_pathname_composition()\n #endif\n \n@@ -270,7 +272,9 @@ struct itimerval {\n #endif\n \n #ifdef NO_SETITIMER\n-#define setitimer(which,value,ovalue)\n+static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) {\n+\t; /* nothing */\n+}\n #endif\n \n #ifndef NO_LIBGEN_H\n@@ -1231,8 +1235,14 @@ int warn_on_fopen_errors(const char *path);\n #endif\n \n #ifndef _POSIX_THREAD_SAFE_FUNCTIONS\n-#define flockfile(fh)\n-#define funlockfile(fh)\n+static inline void flockfile(FILE *fh)\n+{\n+\t; /* nothing */\n+}\n+static inline void funlockfile(FILE *fh)\n+{\n+\t; /* nothing */\n+}\n #define getc_unlocked(fh) getc(fh)\n #endif\n \n"},{"id":"403110","messageId":"20200807013230.GD8085@camp.crustytoothpaste.net","threadId":"54005","inReplyTo":"xmqqpn83i9sk.fsf@gitster.c.googlers.com","subject":"Re: \"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-08-07T01:32:30Z","receivedAt":"2020-08-07T01:33:09Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-08-07 at 00:23:07, Junio C Hamano wrote:\n> While this guarantees that no unneeded code is generated, it also\n> discards type and other checks on these parameters, e.g. a new code\n> written with the argv-array API (diff_args is of type \"struct\n> argv_array\" that has .argc and .argv members):\n> \n>     precompose_argv(diff_args.argc, diff_args.argv);\n> \n> must be updated to use \"struct strvec diff_args\" with .nr and .v\n> members, like so:\n> \n>     precompose_argv(diff_args.nr, diff_args.v);\n> \n> after the argv-array API has been updated to the strvec API.\n> However, the \"no oop\" C preprocessor macro is too aggressive to\n\nMaybe \"no op\" or no-op?\n\n> discard what is unused, and did not catch such a call that was left\n> unconverted.\n> \n> Using a \"static inline\" function whose body is a no-op should still\n> result in the same binary with decent compilers yet catch such a\n> reference to a missing field or passing a value of a wrong type.\n> \n> While at it, I notice that precompute_str() has never been used\n> anywhere in the code, since it was introduced at 76759c7d (git on\n> Mac OS and precomposed unicode, 2012-07-08).  Instead of turning it\n> into a static inline, just remove it.\n\nGreat.  I was wondering about that when I looked at the patch.  If we're\nnot using it, no point in keeping it.  I think the name should be\n\"precompose_str\", though.\n\n> ---\n>  git-compat-util.h | 20 +++++++++++++++-----\n>  1 file changed, 15 insertions(+), 5 deletions(-)\n> \n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 5637114b8d..7a0fb7a045 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -252,8 +252,10 @@ typedef unsigned long uintptr_t;\n>  #ifdef PRECOMPOSE_UNICODE\n>  #include \"compat/precompose_utf8.h\"\n>  #else\n> -#define precompose_str(in,i_nfd2nfc)\n> -#define precompose_argv(c,v)\n> +static inline void precompose_argv(int argc, const char **argv)\n> +{\n> +\t; /* nothing */\n> +}\n>  #define probe_utf8_pathname_composition()\n>  #endif\n>  \n> @@ -270,7 +272,9 @@ struct itimerval {\n>  #endif\n>  \n>  #ifdef NO_SETITIMER\n> -#define setitimer(which,value,ovalue)\n> +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) {\n\nThe rest of the patch looks fine, but do we know that these structs will\nexist if NO_SETITIMER is defined?  If not, we may need to use a void *\nhere, which would provide us worse type checking, but would work on\nplatforms that lack the interval timers at all, such as NonStop.\n\nThat does kind of defeat the purpose of this patch, but I still think\nit's a win, since we end up with some type checking, even if it's not\nperfect, and almost every platform provides setitimer, so any errors\nwill be caught quickly.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"403112","messageId":"20200807032723.GA15119@coredump.intra.peff.net","threadId":"54005","inReplyTo":"xmqqpn83i9sk.fsf@gitster.c.googlers.com","subject":"Re: \"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T03:27:23Z","receivedAt":"2020-08-07T03:27:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 06, 2020 at 05:23:07PM -0700, Junio C Hamano wrote:\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 5637114b8d..7a0fb7a045 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -252,8 +252,10 @@ typedef unsigned long uintptr_t;\n>  #ifdef PRECOMPOSE_UNICODE\n>  #include \"compat/precompose_utf8.h\"\n>  #else\n> -#define precompose_str(in,i_nfd2nfc)\n> -#define precompose_argv(c,v)\n> +static inline void precompose_argv(int argc, const char **argv)\n> +{\n> +\t; /* nothing */\n> +}\n\nThis one looks safe and seems like an improvement, since it's our own\nfunction that we're redefining.\n\nBut this one for example:\n\n> -#define flockfile(fh)\n> -#define funlockfile(fh)\n> +static inline void flockfile(FILE *fh)\n> +{\n> +\t; /* nothing */\n> +}\n> +static inline void funlockfile(FILE *fh)\n> +{\n> +\t; /* nothing */\n> +}\n\nis re-defining a name that's usually reserved for the system (at least\non POSIX systems). For most systems that define it, we'd actually use\nthe system version (and not compile this code at all). But there may be\nsystems where we choose not to (either the system version is deficient,\nor we're testing the fallback code on a more-capable system, or our\n#ifndef check isn't sufficient on that system for some reason).\n\nIf the system defines it as a macro, we'd probably get a garbled\ncompiler error as the macro is expanded here. Adding #undef flockfile,\netc beforehand would help. I'm not sure if the current code might give\nus a macro redefinition warning on such a system.\n\nIf the system defines it as a function, we'd probably get redefinition\nwarnings.\n\nSo...I dunno. Those are all theoretical complaints. But I also think\nwhat it's buying is not very big:\n\n  - unlike precompose_argv(), modern POSIX-compliant systems (which we\n    all tend to develop on) don't hit this fallback code. So your\n    average developer is likely to see any problems here.\n\n  - this is really the tip of the portability iceberg anyway. In the\n    example that motivated this thread, you were catching failures to\n    adjust to strvec. But in code like this:\n\n      #ifdef FOO\n      void some_func(int x, const char *y)\n      {\n              struct argv_array whatever = ARGV_ARRAY_INIT;\n\t      ...\n      }\n      #else\n      void some_func(int x, const char *y)\n      {\n              /* do nothing */\n      }\n      #endif\n\n    You'd still fail to notice the problem if you're not compiling with\n    -DFOO. I think we have to accept that the compiler on a given\n    platform won't be able to catch everything. That's why we have CI on\n    many platforms, plus people building and reporting problems on their\n    own systems.\n\n> @@ -270,7 +272,9 @@ struct itimerval {\n>  #endif\n>  \n>  #ifdef NO_SETITIMER\n> -#define setitimer(which,value,ovalue)\n> +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) {\n> +\t; /* nothing */\n> +}\n>  #endif\n\nSame reasoning applies to this one, plus the added bonus that we'd need\nthat struct type defined. brian mentioned using \"void *\". That works as\nlong as the system doesn't define setitimer() itself with the real\ntypes. Another option is to forward declare \"struct itimerval\" (which\n_should_ be OK even if the system does so, since our forward declaration\nhas no details). But again, I'm not sure if it's worth the hassle.\n\n-Peff\n"},{"id":"403114","messageId":"xmqqlfirhzl2.fsf@gitster.c.googlers.com","threadId":"54005","inReplyTo":"20200807013230.GD8085@camp.crustytoothpaste.net","subject":"Re: \"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T04:03:37Z","receivedAt":"2020-08-07T04:03:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n>>  #ifdef NO_SETITIMER\n>> -#define setitimer(which,value,ovalue)\n>> +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) {\n>\n> The rest of the patch looks fine, but do we know that these structs will\n> exist if NO_SETITIMER is defined?  If not, we may need to use a void *\n> here, which would provide us worse type checking, but would work on\n> platforms that lack the interval timers at all, such as NonStop.\n\nI thought about that and also making s/FILE/void/ for flockfile()\nand funlockfile() for the same reason.  Indeed my first draft used\n\"void *\".\n\nBut because these no-op macros are designed to be used in the main\ncodepath WITHOUT surrounding #ifdef...#endif for readability, the\nplatforms that use NO_SETITIMER has to declare the variable that the\ncalling site of setitimer() passes as its parameters, so they must\nhave something called \"struct itimerval\".  That is why I ended up\nusing the real type here\n\nFor example, builtin/log.c defines\n\n    static struct itimerval early_output_timer;\n\nand makes an unconditional call OUTSIDE any #ifdef...#endif to\nsetitimer(), like so:\n\n\tearly_output_timer.it_value.tv_sec = 0;\n\tearly_output_timer.it_value.tv_usec = 500000;\n\tsetitimer(ITIMER_REAL, &early_output_timer, NULL);\n\nI would expect that this is the use pattern any users of these\nfallback definitions in git-compat-util.h should follow; those who\ndo not have \"struct itimerval\" natively indeed are using a fallback\ndefinition from <git-compat-util.h>.\n\n> That does kind of defeat the purpose of this patch, but I still think\n> it's a win, since we end up with some type checking, even if it's not\n> perfect, and almost every platform provides setitimer, so any errors\n> will be caught quickly.\n\nYes, even if we loosen the type to \"void *\", it does catch certain\nerrors.  One thing I wrote in the log message is that moving to\n\"static inline\" allows us to catch not just type mismatches but also\nmissing variables (i.e. the original code used a variable that has\nbeen renamed, and the instances of the variable used as parameters\nto these no-op macros were left unmodified).  That's not a type\nmismatch but missing identifier.  The motivating example was quite\nsimilar; it was a field renamed but left unadjusted.\n\nThanks.\n\n\n"},{"id":"403115","messageId":"xmqqh7tfhzb1.fsf@gitster.c.googlers.com","threadId":"54005","inReplyTo":"20200807032723.GA15119@coredump.intra.peff.net","subject":"Re: \"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T04:09:38Z","receivedAt":"2020-08-07T04:09:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But this one for example:\n>\n>> -#define flockfile(fh)\n>> -#define funlockfile(fh)\n>> +static inline void flockfile(FILE *fh)\n>> +{\n>> +\t; /* nothing */\n>> +}\n>> +static inline void funlockfile(FILE *fh)\n>> +{\n>> +\t; /* nothing */\n>> +}\n>\n> is re-defining a name that's usually reserved for the system (at least\n> on POSIX systems). For most systems that define it, we'd actually use\n> the system version (and not compile this code at all).\n\n> But there may be\n> systems where we choose not to (either the system version is deficient,\n> or we're testing the fallback code on a more-capable system, or our\n> #ifndef check isn't sufficient on that system for some reason).\n\nHmph, perhaps.  We'll cross that bridge when we need to port to such\na system, and until then, doing this will more easily catch the need\nto cross that bridge, I would imagine.\n\n> If the system defines it as a macro, we'd probably get a garbled\n> compiler error as the macro is expanded here. Adding #undef flockfile,\n> etc beforehand would help. I'm not sure if the current code might give\n> us a macro redefinition warning on such a system.\n>\n> If the system defines it as a function, we'd probably get redefinition\n> warnings.\n>\n> So...I dunno. Those are all theoretical complaints. But I also think\n> what it's buying is not very big:\n>\n>   - unlike precompose_argv(), modern POSIX-compliant systems (which we\n>     all tend to develop on) don't hit this fallback code. So your\n>     average developer is likely to see any problems here.\n\nThat's OK.  I am not particularly interested in systems that has to\nifdef out flockfile() and funlockfile().  I did these two primarily\nto reduce the number of instances of the bad pattern to implement a\nno-op replacement as C-preprocessor macro that can be copied by less\nexperienced developers.\n\n>   - this is really the tip of the portability iceberg anyway. In the\n>     example that motivated this thread, you were catching failures to\n>     adjust to strvec. But in code like this:\n>\n>       #ifdef FOO\n>       void some_func(int x, const char *y)\n>       {\n>               struct argv_array whatever = ARGV_ARRAY_INIT;\n> \t      ...\n>       }\n>       #else\n>       void some_func(int x, const char *y)\n>       {\n>               /* do nothing */\n>       }\n>       #endif\n\nYes, of course, but as I wrote in my response to Brian, the whole\npoint of using these replacement implementation macros is so that we\ndo not have to sprinkle the main code with such #ifdef/#endif, so\nI think the code like that is what needs to be corrected ;-)\n\n>> @@ -270,7 +272,9 @@ struct itimerval {\n>>  #endif\n>>  \n>>  #ifdef NO_SETITIMER\n>> -#define setitimer(which,value,ovalue)\n>> +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) {\n>> +\t; /* nothing */\n>> +}\n>>  #endif\n>\n> Same reasoning applies to this one, plus the added bonus that we'd need\n> that struct type defined. brian mentioned using \"void *\".\n\nSee my message to Brian.\n"},{"id":"403117","messageId":"20200807043436.GA21829@coredump.intra.peff.net","threadId":"54005","inReplyTo":"xmqqh7tfhzb1.fsf@gitster.c.googlers.com","subject":"Re: \"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T04:34:36Z","receivedAt":"2020-08-07T04:34:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 06, 2020 at 09:09:38PM -0700, Junio C Hamano wrote:\n\n> > But there may be\n> > systems where we choose not to (either the system version is deficient,\n> > or we're testing the fallback code on a more-capable system, or our\n> > #ifndef check isn't sufficient on that system for some reason).\n> \n> Hmph, perhaps.  We'll cross that bridge when we need to port to such\n> a system, and until then, doing this will more easily catch the need\n> to cross that bridge, I would imagine.\n\nI agree my concern is theoretical. It might not bite us. If you're\nwilling to deal with it when it does (and inevitably as maintainer you\nwill ;) ), then I'm OK with trying it out.\n\n> >   - this is really the tip of the portability iceberg anyway. In the\n> >     example that motivated this thread, you were catching failures to\n> >     adjust to strvec. But in code like this:\n> >\n> >       #ifdef FOO\n> >       void some_func(int x, const char *y)\n> >       {\n> >               struct argv_array whatever = ARGV_ARRAY_INIT;\n> > \t      ...\n> >       }\n> >       #else\n> >       void some_func(int x, const char *y)\n> >       {\n> >               /* do nothing */\n> >       }\n> >       #endif\n> \n> Yes, of course, but as I wrote in my response to Brian, the whole\n> point of using these replacement implementation macros is so that we\n> do not have to sprinkle the main code with such #ifdef/#endif, so\n> I think the code like that is what needs to be corrected ;-)\n\nI'm not sure if I made my point clearly. At some point you have to\ndefine the functions, and they'll have code in them which gets compiled\non some systems but not on others. E.g., precompose_argv() uses\nreencode_string_iconv(). What if we change the signature of that\nfunction? You will not ever catch it by compiling on your Linux box, and\nwould only see it in macOS CI, etc.\n\n-Peff\n"},{"id":"403118","messageId":"xmqqd043hs7w.fsf@gitster.c.googlers.com","threadId":"54005","inReplyTo":"20200807043436.GA21829@coredump.intra.peff.net","subject":"Re: \"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T06:42:43Z","receivedAt":"2020-08-07T06:42:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Yes, of course, but as I wrote in my response to Brian, the whole\n>> point of using these replacement implementation macros is so that we\n>> do not have to sprinkle the main code with such #ifdef/#endif, so\n>> I think the code like that is what needs to be corrected ;-)\n>\n> I'm not sure if I made my point clearly. ...\n\nSorry, indeed I missed it.  \n\nUsing the implementation of precompose_argv() itself as an example,\ninstead of generic looking some_func(), does help to explain why at\nsome point we are bound to have some API calls that are compiled on\nsome platforms and not on others.\n\n"},{"id":"403120","messageId":"20200807075642.cw6zjym7xykxbsdm@tb-raspi4","threadId":"54005","inReplyTo":"xmqqy2mribft.fsf@gitster.c.googlers.com","subject":"Re: \"#define precompose_argv(c,v) /* empty */\" is evil","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2020-08-07T07:56:43Z","receivedAt":"2020-08-07T07:56:57Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Thu, Aug 06, 2020 at 04:47:34PM -0700, Junio C Hamano wrote:\n> I had seen an interesting compilation breakage today.  A topic adds\n> many more uses of argv-array API so I resolved semantic conflict\n> patch until \"make builtins/submodule-helper.o\" passed.  I failed to\n> spot one remaining breakage until I saw\n>\n>     https://travis-ci.org/github/git/git/jobs/715668996\n>\n> The problematic line was\n>\n>     precompose_argv(diff_args.argc, diff_args.argv);\n>\n> where diff_args used to be an argv_array and is now a strvec.\n>\n> Why didn't I catch this in my local test?\n>\n> $ git grep -n -e precompose_argv -- \\*.h\n> compat/precompose_utf8.h:31:void precompose_argv(int argc, const char **argv);\n> git-compat-util.h:256:#define precompose_argv(c,v)\n>\n> The problematic part is this one used on all platforms other than macOS:\n>\n>     /* used on Mac OS X */\n>     #ifdef PRECOMPOSE_UNICODE\n>     #include \"compat/precompose_utf8.h\"\n>     #else\n>     #define precompose_str(in,i_nfd2nfc)\n>     #define precompose_argv(c,v)\n>     #define probe_utf8_pathname_composition()\n>     #endif\n>\n> I am wondering if it is a good idea to use something like\n>\n>     static inline void precompose_argv(int argc, const char **argv)\n>     {\n> \t; /* nothing */\n>     }\n>\n> instead.  As long as the compiler is reasonable enough, this should\n> not result in any code change in the result, except that it would\n> still catch wrong arguments, even if these two parameters are unused\n> and optimized out.\n>\n\nIn my every-day-live these kind of \"late detection of breakage\" is\nnot uncommon.\nIn other words: this patch allows early detection and is much better.\n\nThanks for cleaning up my mess.\n"}]}