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

From: Sebastian Schuberth <sschuberth@gmail.com>
Date: Sep 13, 2013, 12:47 UTC
Message-ID: <CAHGBnuOQ-y1beD_X_jiH+FrhPvLOVJqT0J=Wk988Q4NeCs1-9Q@mail.gmail.com>
In-Reply-To: <xmqqr4ctokat.fsf@gitster.dls.corp.google.com>

On Thu, Sep 12, 2013 at 10:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> 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.

Well, if by "everywhere" in (1) you mean "on all platforms" then you're right. But my patch does not define __NO_INLINE__ globally, but only at the time string.h / strings.h is included. Afterwards __NO_INLINE__ is undefined. In that sense, __NO_INLINE__ is not defined "everywhere".

> Agreed.  If that #define __NO_INLINE__ does not appear in the common
> part of our header files like git-compat-util.h but is limited to
> somewhere in compat/, that would be the perfect outcome.

It's not that easy to move the definition of __NO_INLINE__ into compat/ because git-compat-util.h includes string.h / strings.h before anything of compat/. More over, defining __NO_INLINE__ in somewhere in compat/ would not limit its definition to the string.h / strings.h headers only. So how about something like this on top of my original patch:

--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -85,12 +85,16 @@
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1

+#ifdef __MINGW32__
 #define __NO_INLINE__ /* do not inline strcasecmp() */
+#endif
 #include <string.h>
+#ifdef __MINGW32__
+#undef __NO_INLINE__
+#endif
 #ifdef HAVE_STRINGS_H
 #include <strings.h> /* for strcasecmp() */
 #endif
-#undef __NO_INLINE__

 #ifdef WIN32 /* Both MinGW and MSVC */
 #ifndef _WIN32_WINNT

-- 
Sebastian Schuberth