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