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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

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

>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]