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

From: Jeff King <peff@peff.net>
Date: Aug 7, 2020, 04:34 UTC
Message-ID: <20200807043436.GA21829@coredump.intra.peff.net>
In-Reply-To: <xmqqh7tfhzb1.fsf@gitster.c.googlers.com>

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

Show 8 quoted lines

> > 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.

Show 21 quoted lines

> >   - 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