[cfe-commits] [PATCH] Improve -Wtautological-constant-out-of-range-compare

[Richard Trieu]

================
Comment at: lib/Sema/SemaChecking.cpp:4343
@@ -4337,2 +4342,3 @@
          && "comparison with non-integer type");
   // FIXME. handle cases for signedness to catch (signed char)N == 200
+
----------------
David Blaikie wrote:
> Does your change address this FIXME? If so, please remove the comment & ensure a relevant test case exists.
Yes, this case is now handled.  Test cases included.

================
Comment at: lib/Sema/SemaChecking.cpp:4371
@@ +4370,3 @@
+    } else if (!OtherSigned && ConstantSigned) {
+      if (S.Context.getIntWidth(OtherT) >= Value.getActiveBits())
+        return;
----------------
David Blaikie wrote:
> This & the condition on line 4368 are the same, aren't they? So this could just be written as:
> 
> if (!OtherSigned) {
>   if (S.Context.getIntWidth(OtherT) >= Value.getActiveBits())
>     return;
> } else {
>   assert(!ConstantSigned && "Two signed types convert to unsigned types.")
>   // Check to see if the constant is representable in OtherT.
>  ...
> }
Done

================
Comment at: test/Analysis/additive-folding.cpp:135
@@ -134,3 +134,3 @@
   clang_analyzer_eval(a != 0x100); // expected-warning{{comparison of constant 256 with expression of type 'unsigned char' is always true}} expected-warning{{TRUE}}
-  clang_analyzer_eval(a != -1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(a != -1); // expected-warning{{comparison of constant -1 with expression of type 'unsigned char' is always true}} expected-warning{{TRUE}}
 
----------------
David Blaikie wrote:
> What is the other warning firing on this line? (the one checked by "TRUE") I guess that's from the analyzer hook & not a redundant warning we need to worry about?
That is correct.  The "TRUE" comes from the analyzer and not from this warning.

================
Comment at: test/SemaCXX/compare.cpp:230
@@ +229,3 @@
+  const unsigned B = -1;
+  void (s < A); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
+  void (s > A); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
----------------
David Blaikie wrote:
> These "comparison of integers of different signs" are not new warnings found by your change, are they? They're just negative tests to ensure we don't claim that these are tautological? (I would suggest disabling this warning, but I assume, judging by the source file name, that this diagnostic is tested elsewhere in the same file)
> 
> It's possible this warning needs a different description/subflag here, too. In this case the comparison will promote the short to unsigned int, won't it? Thus stripping the negative values. (but in, say, unsigned int < int the unsigned int will be converted to int (wrapping around or whatever that causes for high unsigned values outside the range of int) which is a bit different) - it'd be useful to explain to the user what's going on, maybe.
I've updated the test and added new comments to clarify what is going on.


http://llvm-reviews.chandlerc.com/D113