Jeff King [off-list ref] 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.
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.
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.
Thanks, both.
On Thu, Sep 12, 2013 at 10:29 PM, Junio C Hamano [off-list ref] wrote:
Jeff King [off-list ref] writes:
quoted
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