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

[David Blaikie]

================
Comment at: lib/Sema/SemaChecking.cpp:4371
@@ +4370,3 @@
+    } else if (!OtherSigned && ConstantSigned) {
+      if (S.Context.getIntWidth(OtherT) >= Value.getActiveBits())
+        return;
----------------
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.
 ...
}

================
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}}
 
----------------
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?

================
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'}}
----------------
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.

================
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
+
----------------
Does your change address this FIXME? If so, please remove the comment & ensure a relevant test case exists.


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