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

[Eli Friedman]

Comments below.

On Mon, Nov 12, 2012 at 9:46 AM, Richard Trieu <rtrieu at google.com> wrote:
> Make -Wtautological-constant-out-of-range-compare checking take into account types and conversion between types.  The old version merely checked the bit widths, which allowed failed to catch a few cases, while warning on other safe comparisons.
>
> http://llvm-reviews.chandlerc.com/D113
>
> Files:
>   test/Analysis/additive-folding.cpp
>   test/SemaCXX/compare.cpp
>   test/SemaCXX/warn-enum-compare.cpp
>   lib/Sema/SemaChecking.cpp

Index: test/SemaCXX/warn-enum-compare.cpp
===================================================================
--- test/SemaCXX/warn-enum-compare.cpp
+++ test/SemaCXX/warn-enum-compare.cpp
@@ -39,8 +39,8 @@
   while (b == c);
   while (B1 == name1::B2);
   while (B2 == name2::B1);
-  while (x == AnonAA); // expected-warning {{comparison of constant
42 with expression of type 'Foo' is always false}}
-  while (AnonBB == y); // expected-warning {{comparison of constant
45 with expression of type 'Bar' is always false}}
+  while (x == AnonAA);
+  while (AnonBB == y);
   while (AnonAA == AnonAB);
   while (AnonAB == AnonBA);
   while (AnonBB == AnonAA);

Why are you changing this warning?

   while (AnonBB == AnonAA);
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -4328,38 +4328,91 @@
                                          Expr *Constant, Expr *Other,
                                          llvm::APSInt Value,
                                          bool RhsConstant) {
+  // 0 values are handled later by CheckTrivialUnsignedComparison().
+  if (Value == 0)
+    return;
+
   BinaryOperatorKind op = E->getOpcode();
   QualType OtherT = Other->getType();
   QualType ConstantT = Constant->getType();
+  QualType CommonT = E->getLHS()->getType();
   if (S.Context.hasSameUnqualifiedType(OtherT, ConstantT))
     return;
   assert((OtherT->isIntegerType() && ConstantT->isIntegerType())
          && "comparison with non-integer type");
   // FIXME. handle cases for signedness to catch (signed char)N == 200

Does this patch fix this FIXME?

-  IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
-  IntRange LitRange = GetValueRange(S.Context, Value, Value.getBitWidth());
-  if (OtherRange.Width >= LitRange.Width)
+
+  bool ConstantSigned = ConstantT->isSignedIntegerType();
+  bool OtherSigned = OtherT->isSignedIntegerType();
+  bool CommonSigned = CommonT->isSignedIntegerType();

Why are you getting rid of the IntRange usage?  Granted, we didn't
really have any good testcases, but it does add power to the analysis
in some cases.

+  bool EqualityOnly = false;
+
+  if (CommonSigned) {
+    // The common type is signed, therefore no signed to unsigned conversion.
+    if (OtherSigned) {
+      // Check that the constant is representable in type OtherT.
+      if (S.Context.getIntWidth(OtherT) >= Value.getMinSignedBits())

Calling getMinSignedBits() on an unsigned constant will do the wrong
thing in cases like 0xFFFFFFFFU.  (I'm not sure it's actually possible
to write a testcase where this matters, but it would make more sense
anyway.)

+        return;
+    } else { // !OtherSigned
+      // Check that the constant is representable in type OtherT.
+      // Negative values are out of range.
+      if (Value.isNonNegative() &&
+          S.Context.getIntWidth(OtherT) >= Value.getActiveBits())

Calling isNonNegative() on an unsigned constant can also do the wrong thing.

.+        return;
+    }
+  } else {  // !CommonSigned
+    if (!OtherSigned && !ConstantSigned) {
+      // Both types are unsigned, check the constant is representable in
+      // type OtherT.
+      if (S.Context.getIntWidth(OtherT) >= Value.getActiveBits())
+        return;
+    } else if (!OtherSigned && ConstantSigned) {
+      if (S.Context.getIntWidth(OtherT) >= Value.getActiveBits())
+        return;

Calling getActiveBits() on a signed value is suspicious; the return
will vary with the width of the constant for the same value.

+    } else if (OtherSigned && !ConstantSigned) {
+      // Check to see if the constant is representable in OtherT.
+      if (S.Context.getIntWidth(OtherT) > Value.getActiveBits())
+        return;
+      // Check to see if the constant is equivalent to a negative value
+      // cast to CommonT.
+      if (S.Context.getIntWidth(ConstantT) == S.Context.getIntWidth(CommonT) &&
+          Value.isNegative() &&

Is this going to handle cases like (int)x == 0xFFFFFFFFFFFFFFFFULL correctly?

-Eli