Sebastian Schuberth [off-list ref] writes:
On Tue, Sep 17, 2013 at 6:17 PM, Junio C Hamano [off-list ref] wrote:
quoted
Keeping the ugliness to deal with the platform issue (i.e. broken
string.h) in one place (e.g. compat/mingw) is far more preferrable
than having a similar ugliness in git-compat-util.h for people on
all other platforms to see, no?
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?
Also, your solution does not really keep the ugliness in
one place,...
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 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.
On Tue, Sep 17, 2013 at 11:46 PM, Junio C Hamano [off-list ref] wrote:
quoted
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.
quoted
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
On Wed, Sep 18, 2013 at 5:43 AM, Sebastian Schuberth
[off-list ref] wrote:
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.
So I think it's an annoying MinGW bug, but the reason I dislike the
"no-inline" approach is two-fold:
- it's *way* too intimate with the bug.
When you have a bug like this, the *last* thing you want to do is
to make sweet sweet love to it, and get really involved with it.
You want to say "Eww, what a nasty little bug, I don't want to have
anything to do with you".
And quite frankly, delving into the details of exactly *what* MinGW
does wrong, and defining magic __NO_INLINE__ macros, knowing that that
is the particular incantation that hides the MinGW bug, that's being
too intimate. That's simply a level of detail that *nobody* should
ever have to know.
The other patch (having just a wrapper function) doesn't have those
kinds of intimacy issues. That patch just says "MinGW is buggy and
cannot do this function uninlined, so we wrap it". Notice the lack of
detail, and lack of *interest* in the exact particular pattern of the
bug.
The other reason I'm not a fan of the __NO_INLINE__ approach is even
more straightforward:
- Why should we disable the inlining of everything in <string.h> (and
possibly elsewhere too - who the hell knows what __NO_INLINE__ will do
to other header files), when in 99% of all the cases we don't care,
and in fact inlining may well be good and the right thing to do.
So the __NO_INLINE__ games seem to be both too big of a hammer, and
too non-specific, and at the same time it gets really intimate with
MinGW in unhealthy ways.
If you know something is diseased, you keep your distance, you don't
try to embrace it.
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?
Ugh, so now that patch is fragile, so we have to complicate it even more.
Really, just make a wrapper function. It doesn't even need to be
conditional on MinGW. Just a single one-liner function, with a comment
above it that says "MinGW is broken and doesn't have an out-of-line
copy of strcasecmp(), so we wrap it here".
No unnecessary details about internal workings of a buggy MinGW header
file. No complexity. No subtle issues with include file ordering. Just
a straightforward workaround that is easy to explain.
Linus