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

From: Jeff King <peff@peff.net>
Date: Sep 12, 2013, 20:22 UTC
Message-ID: <20130912202246.GF32069@sigill.intra.peff.net>
In-Reply-To: <CAHGBnuPzzokV7YMrx0gAL1VACcmaLwFoaB3n6bX8Y-UDHs7S8A@mail.gmail.com>

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

Show 17 quoted lines

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