Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
On Thu, Jan 17, 2013 at 11:44 AM, Linus Torvalds torva...@linux-foundation.org wrote: On Thu, Jan 17, 2013 at 3:00 AM, John Keeping j...@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. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
On Fri, Jan 18, 2013 at 9:15 AM, Phil Hord phil.h...@gmail.com wrote: 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. Only theoretically. Git won't work on machines that don't have 8-bit bytes anyway, so worrying about the theoretical crazy architectures that aren't two's complement etc isn't something I'd care about. There's a whole class of technically implementation-defined issues in C that simply aren't worth caring for. Yes, the standard is written so that it works on machines that aren't byte-addressable, or EBCDIC or have things like 18-bit words and 36-bit longwords. Or 16-bit int for microcontrollers etc. That doesn't make those implementation-defined issues worth worrying about these days. A compiler writer could in theory make up some idiotic rules that are still valid by the C standard even on modern machines, but such a compiler should simply not be used, and the compiler writer in question should be called out for being an ass-hat. Paper standards are only worth so much. And that so much really isn't very much. Linus -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
On Thu, Jan 17, 2013 at 3:00 AM, John Keeping j...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
BTW, I think it has been addressed [1] by clang already and that would explain why you don't have the warning when using clang trunk version. [1]: http://llvm-reviews.chandlerc.com/D113 Antoine, On Thu, Jan 17, 2013 at 5:44 PM, Linus Torvalds torva...@linux-foundation.org wrote: On Thu, Jan 17, 2013 at 3:00 AM, John Keeping j...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
On Thu, Jan 17, 2013 at 08:44:20AM -0800, Linus Torvalds wrote: On Thu, Jan 17, 2013 at 3:00 AM, John Keeping j...@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. The warning doesn't occur with a build from their trunk so it looks like it's already fixed - it just won't make into into a release for about 5 months going by their timeline. Thanks for the clear explanation. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
With these two patches and the patch from Max Horne, I'm deeply sorry for this typo Max -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
