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

From: Sebastian Schuberth <sschuberth@gmail.com>
Date: Sep 18, 2013, 09:43 UTC
Message-ID: <CAHGBnuMh9wqe6mhLyqbPAGJUEEH7cA2LZPuCQK8VD=NU2ix3Pg@mail.gmail.com>
In-Reply-To: <xmqqsix3w27t.fsf@gitster.dls.corp.google.com>

On Tue, Sep 17, 2013 at 11:46 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>> I don't think people on other platforms seeing the ugliness is really
>> an issue. After all, the file is called git-*compat*-util.h;
>
> Well, judging from the way Linus reacted to the patch, I'd have to
> disagree.  After all, that argument leads to the position that
> nothing is needed in compat/, no?

My feeling is that Linus' reaction was more about that this work-around is even necessary (and MinGW is buggy) rather than applying it to git-compat-util.h and not elsewhere.

Show 6 quoted lines

> One ugliness (lack of sane strcasecmp definition whose address can
> be taken) specific to mingw is worked around in compat/mingw.h, and
> another ugliness that some people may use compilers without include_next
> may need help from another configuration in the Makefile to tell it
> where the platform string.h resides.  I am not sure why you see it
> as a problem.

I just don't like that the ugliness is spreading out and requires a change to config.mak.uname now, too. Also, I regard the change to config.mak.uname by itself as ugly, mainly because you would have to set SYSTEM_STRING_H_HEADER to some path, but that path might differ from system to system, depending on where MinGW is installed on Windows.

Show 5 quoted lines

>> I do insist to avoid GCC-ism in C files,...
>
> To that I tend to agree.  Unconditionally killing inlining for any
> mingw compilation in compat/mingw.h may be the simplest (albeit it
> may be less than optimal) solution.

I tried to put the __NO_INLINE__ stuff in compat/mingw.h but failed, it involved the need to shuffle includes in git-compat-util.h around because winsock2.h already seems to include string.h, and I did not find a working include order. So I came up with the following, do you like that better?

diff --git a/compat/string_no_inline.h b/compat/string_no_inline.h
new file mode 100644
index 0000000..51eed52
--- /dev/null
+++ b/compat/string_no_inline.h
@@ -0,0 +1,25 @@
+#ifndef STRING_NO_INLINE_H
+#define STRING_NO_INLINE_H
+
+#ifdef __MINGW32__
+#ifdef __NO_INLINE__
+#define __NO_INLINE_ALREADY_DEFINED
+#else
+#define __NO_INLINE__ /* do not inline strcasecmp() */
+#endif
+#endif
+
+#include <string.h>
+#ifdef HAVE_STRINGS_H
+#include <strings.h> /* for strcasecmp() */
+#endif
+
+#ifdef __MINGW32__
+#ifdef __NO_INLINE_ALREADY_DEFINED
+#undef __NO_INLINE_ALREADY_DEFINED
+#else
+#undef __NO_INLINE__
+#endif
+#endif
+
+#endif
diff --git a/git-compat-util.h b/git-compat-util.h
index db564b7..348dd55 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -85,6 +85,8 @@
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1

+#include "compat/string_no_inline.h"
+
 #ifdef WIN32 /* Both MinGW and MSVC */
 #ifndef _WIN32_WINNT
 #define _WIN32_WINNT 0x0502
@@ -101,10 +103,6 @@
 #include <stddef.h>
 #include <stdlib.h>
 #include <stdarg.h>
-#include <string.h>
-#ifdef HAVE_STRINGS_H
-#include <strings.h> /* for strcasecmp() */
-#endif
 #include <errno.h>
 #include <limits.h>
 #ifdef NEEDS_SYS_PARAM_H

-- 
1.8.3.mingw.1.dirty

-- 
Sebastian Schuberth