Re: "#define precompose_argv(c,v) /* empty */" is evil

From: Jeff King <peff@peff.net>
Date: Aug 7, 2020, 03:27 UTC
Message-ID: <20200807032723.GA15119@coredump.intra.peff.net>
In-Reply-To: <xmqqpn83i9sk.fsf@gitster.c.googlers.com>

On Thu, Aug 06, 2020 at 05:23:07PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 5637114b8d..7a0fb7a045 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -252,8 +252,10 @@ typedef unsigned long uintptr_t;
>  #ifdef PRECOMPOSE_UNICODE
>  #include "compat/precompose_utf8.h"
>  #else
> -#define precompose_str(in,i_nfd2nfc)
> -#define precompose_argv(c,v)
> +static inline void precompose_argv(int argc, const char **argv)
> +{
> +	; /* nothing */
> +}

This one looks safe and seems like an improvement, since it's our own function that we're redefining.

But this one for example:

Show 10 quoted lines

> -#define flockfile(fh)
> -#define funlockfile(fh)
> +static inline void flockfile(FILE *fh)
> +{
> +	; /* nothing */
> +}
> +static inline void funlockfile(FILE *fh)
> +{
> +	; /* nothing */
> +}

is re-defining a name that's usually reserved for the system (at least on POSIX systems). For most systems that define it, we'd actually use the system version (and not compile this code at all). But there may be systems where we choose not to (either the system version is deficient, or we're testing the fallback code on a more-capable system, or our #ifndef check isn't sufficient on that system for some reason).

If the system defines it as a macro, we'd probably get a garbled compiler error as the macro is expanded here. Adding #undef flockfile, etc beforehand would help. I'm not sure if the current code might give us a macro redefinition warning on such a system.

If the system defines it as a function, we'd probably get redefinition warnings.

So...I dunno. Those are all theoretical complaints. But I also think what it's buying is not very big:

  - unlike precompose_argv(), modern POSIX-compliant systems (which we
    all tend to develop on) don't hit this fallback code. So your
    average developer is likely to see any problems here.

  - this is really the tip of the portability iceberg anyway. In the
    example that motivated this thread, you were catching failures to
    adjust to strvec. But in code like this:

      #ifdef FOO
      void some_func(int x, const char *y)
      {
              struct argv_array whatever = ARGV_ARRAY_INIT;
	      ...
      }
      #else
      void some_func(int x, const char *y)
      {
              /* do nothing */
      }
      #endif

    You'd still fail to notice the problem if you're not compiling with
    -DFOO. I think we have to accept that the compiler on a given
    platform won't be able to catch everything. That's why we have CI on
    many platforms, plus people building and reporting problems on their
    own systems.

Show 9 quoted lines

> @@ -270,7 +272,9 @@ struct itimerval {
>  #endif
>  
>  #ifdef NO_SETITIMER
> -#define setitimer(which,value,ovalue)
> +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) {
> +	; /* nothing */
> +}
>  #endif

Same reasoning applies to this one, plus the added bonus that we'd need that struct type defined. brian mentioned using "void *". That works as long as the system doesn't define setitimer() itself with the real types. Another option is to forward declare "struct itimerval" (which _should_ be OK even if the system does so, since our forward declaration has no details). But again, I'm not sure if it's worth the hassle.

-Peff