Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[Linus Torvalds <torvalds@linux-foundation.org>]

On Thu, Jan 17, 2013 at 3:00 AM, John Keeping <john@keeping.me.uk> wrote:
>
> There's also a warning that triggers with clang 3.2 but not clang trunk, which
> I think is a legitimate warning - perhaps someone who understands integer type
> promotion better than me can explain why the code is OK (patch->score is
> declared as 'int'):
>
> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
>     with expression of type 'int' is always false
>     [-Wtautological-constant-out-of-range-compare]
>         if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
>             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~

The warning seems to be very very wrong, and implies that clang has
some nasty bug in it.

Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the
conversion rules for the comparison is that the int result from the
assignment is cast to unsigned long. And if you cast (int)-1 to
unsigned long, you *do* get ULONG_MAX. That's true regardless of
whether "long" has the same number of bits as "int" or is bigger. The
implicit cast will be done as a sign-extension (unsigned long is not
signed, but the source type of 'int' *is* signed, and that is what
determines the sign extension on casting).

So the "is always false" is pure and utter crap. clang is wrong, and
it is wrong in a way that implies that it actually generates incorrect
code. It may well be worth making a clang bug report about this.

That said, clang is certainly understandably confused. The code
depends on subtle conversion rules and bit patterns, and is clearly
very confusingly written.

So it would probably be good to rewrite it as

    unsigned long val = strtoul(line, NULL, 10);
    if (val == ULONG_MAX) ..
    patch->score = val;

instead. At which point you might as well make the comparison be ">=
INT_MAX" instead, since anything bigger than that is going to be
bogus.

So the git code is probably worth cleaning up, but for git it would be
a cleanup. For clang, this implies a major bug and bad code
generation.

                   Linus
                     Linus