Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined

On Thu, Sep 12, 2013 at 09:46:51PM +0200, Sebastian Schuberth wrote:

quoted

Right, option 3 seems perfectly reasonable to me, as we must be prepared
to cope with a decision not to inline the function, and there has to be
_some_ linked implementation. But shouldn't libc be providing an
external, linkable strcasecmp in this case?

MinGW / GCC is not linking against libc, but against MSVCRT, Visual
Studio's C runtime. And in fact MSVCRT has a non-inline implementation
of a "case-insensitive string comparison for up to the first n
characters"; it just happens to be called "_strnicmp", not
"strncasecmp". Which is why I still think just having a "#define
strncasecmp _strnicmp" is the most elegant solution to the problem.
And that's exactly what defining __NO_INLINE__ does. Granted, defining
__NO_INLINE__ in the scope of string.h will also add a "#define
strcasecmp _stricmp"; but despite it's name, defining __NO_INLINE__
does not imply a performance hit due to functions not being inlined
because it's just the "strncasecmp" wrapper around "_strnicmp" that's
being inlined, not "_strnicmp" itself.

Ah, thanks, that explains what is going on. I do think the environment
is probably in violation of C99, but I dug in the mingw history, and it
looks like it has been this way for over 10 years.

So it is probably worth working around, but it would be nice if the
damage could be contained to just the affected platform.

I think there are basically three classes of solution:

  1. Declare __NO_INLINE__ everywhere. I'd worry this might affect other
     environments, who would then not inline and lose performance (but
     since it's a non-standard macro, we don't really know what it will
     do in other places; possibly nothing).

  2. Declare __NO_INLINE__ on mingw. Similar to above, but we know it
     only affects mingw, and we know the meaning of NO_INLINE there.

  3. Try to impact only the uses as a function pointer (e.g., by using
     a wrapper function as suggested in the thread).

Your patch does (1), I believe. Junio's patch does (3), but is a
maintenance burden in that any new callsites will need to remember to do
the same trick.

But your argument (and reading the mingw header, I agree) is that there
is no performance difference at all between (2) and (3). And (2) does
not have the maintenance burden. So it does seem like the right path to
me.

-Peff