Thread (2 messages) 2 messages, 2 authors, 2020-08-07

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

From: Jeff King <hidden>
Date: 2020-08-07 04:34:40

On Thu, Aug 06, 2020 at 09:09:38PM -0700, Junio C Hamano wrote:
quoted
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.
quoted
  - 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help