From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 04 Oct 2010 17:54:55 GMT
Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning
Message-ID: <7v8w2d972o.fsf@alter.siamese.dyndns.org>
In-Reply-To: <AANLkTimBzC7f7teH8Rriykg7o5kwX6mD2g2q-0yOXSiF@mail.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Oh it's a bug in NetBSD, sorry for not being explicit about that:
>
>     $ grep S_ISREG /usr/include/sys/stat.h
>     #define S_ISREG(m)      ((m & _S_IFMT) == _S_IFREG)     /* regular file */
>
>     $ grep S_ISREG /usr/include/linux/stat.h
>     #define S_ISREG(m)      (((m) & S_IFMT) == S_IFREG)
>
> I.e. GCC sees `S_IFREG | 0644 & _S_IFMT' on NetBSD but `(S_IFREG |
> 0644) & _S_IFMT' on Linux.
>
> Since bitwise AND (&) has precedence over bitwise OR it's probably a
> logic error on NetBSD too, not just an annoying warning.

In that case, I'd prefer to work this around at the definition of
canon_mode(), like

    /*
     * extra ()-pair around S_ISREG() and friends to work around platform
     * header with buggy definitions like
     * #define S_ISREG(x) ((x & _S_IFMT) == _S_IFREG)
     */
    #define canon_mode(mode) \
    	(S_ISREG((mode)) ? (S_IFREG | ce_permissions(mode)) : \
        ...

instead of contaminating the calling sites.  Otherwise new calling sites
we will add in the future need to be aware of the same bug for no good
reason.