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

[Jeff King <peff@peff.net>]

On Thu, Aug 06, 2020 at 09:09:38PM -0700, Junio C Hamano wrote:

> > 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).
> 
> Hmph, perhaps.  We'll cross that bridge when we need to port to such
> a system, and until then, doing this will more easily catch the need
> to cross that bridge, I would imagine.

I agree my concern is theoretical. It might not bite us. If you're
willing to deal with it when it does (and inevitably as maintainer you
will ;) ), then I'm OK with trying it out.

> >   - 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
> 
> Yes, of course, but as I wrote in my response to Brian, the whole
> point of using these replacement implementation macros is so that we
> do not have to sprinkle the main code with such #ifdef/#endif, so
> I think the code like that is what needs to be corrected ;-)

I'm not sure if I made my point clearly. At some point you have to
define the functions, and they'll have code in them which gets compiled
on some systems but not on others. E.g., precompose_argv() uses
reencode_string_iconv(). What if we change the signature of that
function? You will not ever catch it by compiling on your Linux box, and
would only see it in macOS CI, etc.

-Peff