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

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

 



On Thu, Jan 17, 2013 at 11:44 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Jan 17, 2013 at 3:00 AM, John Keeping <john@xxxxxxxxxxxxx> 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.


Yes, I can tell by the wording of the error message that you are right
and clang has a problem.  But the git code it complained about does
have a real problem, because the result of "signed int a = ULONG_MAX"
is implementation-defined.  It cannot be guaranteed or expected that
patch->score will ever be assigned -1 there, and so the comparison may
always be false.  I guess the warning is correct, but only
accidentally.  :-)

Your rewrite is more sane and corrects the problem, I think.

Phil
--
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]