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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sep 18, 2013, 12:19 UTC
Message-ID: <CA+55aFwJQ7yo3N3rdAz2=o9Zxxt4ascF5kzvB6-YHL+HXbz7ug@mail.gmail.com>
In-Reply-To: <CAHGBnuMh9wqe6mhLyqbPAGJUEEH7cA2LZPuCQK8VD=NU2ix3Pg@mail.gmail.com>

On Wed, Sep 18, 2013 at 5:43 AM, Sebastian Schuberth <sschuberth@gmail.com> 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.

Show 5 quoted lines

> 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