[Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

Summary:
This is a first half(?) of a fix for the following bug:
https://bugs.llvm.org/show_bug.cgi?id=34147 (gcc -Wtype-limits)

GCC's -Wtype-limits does warn on comparison of unsigned value
with signed zero (as in, with 0), but clang only warns if the
zero is unsigned (i.e. 0U).

Also, be careful not to double-warn, or falsely warn on
comparison of signed/fp variable and signed 0.

Yes, all these testcases are needed.

Testing: $ ninja check-clang-sema check-clang-semacxx
Also, no new warnings for clang stage-2 build.

Reviewers: rjmccall, rsmith, aaron.ballman

Reviewed By: rjmccall

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D37565

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@312750 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst
index d84fd3a7f4db828d7f870df3428d5a68c5cfa1e2..60d418bd4a01243d056511c7fbaf314f83eb8c87 100644 (file)
--- a/docs/ReleaseNotes.rst
+++ b/docs/ReleaseNotes.rst
@@ -71,6 +71,13 @@ Improvements to Clang's diagnostics
   errors/warnings, as the system frameworks might add a method with the same
   selector which could make the message send to ``id`` ambiguous.
 
+- ``-Wtautological-compare`` now warns when comparing an unsigned integer and 0
+  regardless of whether the constant is signed or unsigned."
+
+- ``-Wtautological-compare`` now warns about comparing a signed integer and 0
+  when the signed integer is coerced to an unsigned type for the comparison.
+  ``-Wsign-compare`` was adjusted not to warn in this case.
+
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 

diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 7b0f72fd998ff32c841781df1752c057a52aee47..212fe65c76f66a3561680616a3f4947bab9afe0d 100644 (file)
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -8567,32 +8567,51 @@ bool HasEnumType(Expr *E) {
   return E->getType()->isEnumeralType();
 }
 
-void CheckTrivialUnsignedComparison(Sema &S, BinaryOperator *E) {
+bool isNonBooleanUnsignedValue(Expr *E) {
+  // We are checking that the expression is not known to have boolean value,
+  // is an integer type; and is either unsigned after implicit casts,
+  // or was unsigned before implicit casts.
+  return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType() &&
+         (!E->getType()->isSignedIntegerType() ||
+          !E->IgnoreParenImpCasts()->getType()->isSignedIntegerType());
+}
+
+bool CheckTautologicalComparisonWithZero(Sema &S, BinaryOperator *E) {
   // Disable warning in template instantiations.
   if (S.inTemplateInstantiation())
-    return;
+    return false;
+
+  // bool values are handled by DiagnoseOutOfRangeComparison().
 
   BinaryOperatorKind op = E->getOpcode();
   if (E->isValueDependent())
-    return;
+    return false;
+
+  Expr *LHS = E->getLHS();
+  Expr *RHS = E->getRHS();
+
+  bool Match = true;
 
-  if (op == BO_LT && IsZero(S, E->getRHS())) {
+  if (op == BO_LT && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) {
     S.Diag(E->getOperatorLoc(), diag::warn_lunsigned_always_true_comparison)
-      << "< 0" << "false" << HasEnumType(E->getLHS())
-      << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
-  } else if (op == BO_GE && IsZero(S, E->getRHS())) {
+      << "< 0" << "false" << HasEnumType(LHS)
+      << LHS->getSourceRange() << RHS->getSourceRange();
+  } else if (op == BO_GE && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) {
     S.Diag(E->getOperatorLoc(), diag::warn_lunsigned_always_true_comparison)
-      << ">= 0" << "true" << HasEnumType(E->getLHS())
-      << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
-  } else if (op == BO_GT && IsZero(S, E->getLHS())) {
+      << ">= 0" << "true" << HasEnumType(LHS)
+      << LHS->getSourceRange() << RHS->getSourceRange();
+  } else if (op == BO_GT && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) {
     S.Diag(E->getOperatorLoc(), diag::warn_runsigned_always_true_comparison)
-      << "0 >" << "false" << HasEnumType(E->getRHS())
-      << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
-  } else if (op == BO_LE && IsZero(S, E->getLHS())) {
+      << "0 >" << "false" << HasEnumType(RHS)
+      << LHS->getSourceRange() << RHS->getSourceRange();
+  } else if (op == BO_LE && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) {
     S.Diag(E->getOperatorLoc(), diag::warn_runsigned_always_true_comparison)
-      << "0 <=" << "true" << HasEnumType(E->getRHS())
-      << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
-  }
+      << "0 <=" << "true" << HasEnumType(RHS)
+      << LHS->getSourceRange() << RHS->getSourceRange();
+  } else
+    Match = false;
+
+  return Match;
 }
 
 void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, Expr *Constant,
@@ -8612,7 +8631,7 @@ void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, Expr *Constant,
 
   bool OtherIsBooleanType = Other->isKnownToHaveBooleanValue();
 
-  // 0 values are handled later by CheckTrivialUnsignedComparison().
+  // 0 values are handled later by CheckTautologicalComparisonWithZero().
   if ((Value == 0) && (!OtherIsBooleanType))
     return;
 
@@ -8849,16 +8868,22 @@ void AnalyzeComparison(Sema &S, BinaryOperator *E) {
         (IsRHSIntegralLiteral && IsLHSIntegralLiteral);
   } else if (!T->hasUnsignedIntegerRepresentation())
       IsComparisonConstant = E->isIntegerConstantExpr(S.Context);
-  
+
+  // We don't care about value-dependent expressions or expressions
+  // whose result is a constant.
+  if (IsComparisonConstant)
+    return AnalyzeImpConvsInComparison(S, E);
+
+  // If this is a tautological comparison, suppress -Wsign-compare.
+  if (CheckTautologicalComparisonWithZero(S, E))
+    return AnalyzeImpConvsInComparison(S, E);
+
   // We don't do anything special if this isn't an unsigned integral
   // comparison:  we're only interested in integral comparisons, and
   // signed comparisons only happen in cases we don't care to warn about.
-  //
-  // We also don't care about value-dependent expressions or expressions
-  // whose result is a constant.
-  if (!T->hasUnsignedIntegerRepresentation() || IsComparisonConstant)
+  if (!T->hasUnsignedIntegerRepresentation())
     return AnalyzeImpConvsInComparison(S, E);
-  
+
   // Check to see if one of the (unmodified) operands is of different
   // signedness.
   Expr *signedOperand, *unsignedOperand;
@@ -8871,7 +8896,6 @@ void AnalyzeComparison(Sema &S, BinaryOperator *E) {
     signedOperand = RHS;
     unsignedOperand = LHS;
   } else {
-    CheckTrivialUnsignedComparison(S, E);
     return AnalyzeImpConvsInComparison(S, E);
   }
 
@@ -8883,11 +8907,9 @@ void AnalyzeComparison(Sema &S, BinaryOperator *E) {
   AnalyzeImplicitConversions(S, LHS, E->getOperatorLoc());
   AnalyzeImplicitConversions(S, RHS, E->getOperatorLoc());
 
-  // If the signed range is non-negative, -Wsign-compare won't fire,
-  // but we should still check for comparisons which are always true
-  // or false.
+  // If the signed range is non-negative, -Wsign-compare won't fire.
   if (signedRange.NonNegative)
-    return CheckTrivialUnsignedComparison(S, E);
+    return;
 
   // For (in)equality comparisons, if the unsigned operand is a
   // constant which cannot collide with a overflowed signed operand,

diff --git a/test/Sema/compare.c b/test/Sema/compare.c
index 887bce06306caf195779c77b0a2d222a9e3f3b78..ae261432dcddad0d073801acd367a4bb082b1a6a 100644 (file)
--- a/test/Sema/compare.c
+++ b/test/Sema/compare.c
@@ -77,7 +77,7 @@ int ints(long a, unsigned long b) {
          ((int) a == (unsigned int) B) +
          ((short) a == (unsigned short) B) +
          ((signed char) a == (unsigned char) B) +
-         (a < (unsigned long) B) +  // expected-warning {{comparison of integers of different signs}}
+         (a < (unsigned long) B) +  // expected-warning {{comparison of unsigned expression < 0 is always false}}
          (a < (unsigned int) B) +
          (a < (unsigned short) B) +
          (a < (unsigned char) B) +
@@ -85,8 +85,8 @@ int ints(long a, unsigned long b) {
          ((int) a < B) +
          ((short) a < B) +
          ((signed char) a < B) +
-         ((long) a < (unsigned long) B) +  // expected-warning {{comparison of integers of different signs}}
-         ((int) a < (unsigned int) B) +  // expected-warning {{comparison of integers of different signs}}
+         ((long) a < (unsigned long) B) +  // expected-warning {{comparison of unsigned expression < 0 is always false}}
+         ((int) a < (unsigned int) B) +  // expected-warning {{comparison of unsigned expression < 0 is always false}}
          ((short) a < (unsigned short) B) +
          ((signed char) a < (unsigned char) B) +
 

diff --git a/test/Sema/outof-range-constant-compare.c b/test/Sema/outof-range-constant-compare.c
index 4b1637c46c5e32a541ff32f865e884ffba4bb4cb..36b6ff69ca78f7997d39d4d2b22c8baed63db999 100644 (file)
--- a/test/Sema/outof-range-constant-compare.c
+++ b/test/Sema/outof-range-constant-compare.c
@@ -6,6 +6,59 @@ int value(void);
 int main()
 {
     int a = value();
+
+    if (a == 0x0000000000000000L)
+        return 0;
+    if (a != 0x0000000000000000L)
+        return 0;
+    if (a < 0x0000000000000000L)
+        return 0;
+    if (a <= 0x0000000000000000L)
+        return 0;
+    if (a > 0x0000000000000000L)
+        return 0;
+    if (a >= 0x0000000000000000L)
+        return 0;
+
+    if (0x0000000000000000L == a)
+        return 0;
+    if (0x0000000000000000L != a)
+        return 0;
+    if (0x0000000000000000L < a)
+        return 0;
+    if (0x0000000000000000L <= a)
+        return 0;
+    if (0x0000000000000000L > a)
+        return 0;
+    if (0x0000000000000000L >= a)
+        return 0;
+
+    if (a == 0x0000000000000000UL)
+        return 0;
+    if (a != 0x0000000000000000UL)
+        return 0;
+    if (a < 0x0000000000000000UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+        return 0;
+    if (a <= 0x0000000000000000UL)
+        return 0;
+    if (a > 0x0000000000000000UL)
+        return 0;
+    if (a >= 0x0000000000000000UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
+        return 0;
+
+    if (0x0000000000000000UL == a)
+        return 0;
+    if (0x0000000000000000UL != a)
+        return 0;
+    if (0x0000000000000000UL < a)
+        return 0;
+    if (0x0000000000000000UL <= a) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
+        return 0;
+    if (0x0000000000000000UL > a) // expected-warning {{comparison of 0 > unsigned expression is always false}}
+        return 0;
+    if (0x0000000000000000UL >= a)
+        return 0;
+
     if (a == 0x1234567812345678L) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'int' is always false}}
         return 0;
     if (a != 0x1234567812345678L) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'int' is always true}}
@@ -74,6 +127,7 @@ int main()
         return 0;
     if (0x1234567812345678L >= s) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always true}}
         return 0;
+
     long l = value();
     if (l == 0x1234567812345678L)
         return 0;
@@ -106,13 +160,13 @@ int main()
         return 0;
     if (un != 0x0000000000000000L)
         return 0;
-    if (un < 0x0000000000000000L)
+    if (un < 0x0000000000000000L) // expected-warning {{comparison of unsigned expression < 0 is always false}}
         return 0;
     if (un <= 0x0000000000000000L)
         return 0;
     if (un > 0x0000000000000000L)
         return 0;
-    if (un >= 0x0000000000000000L)
+    if (un >= 0x0000000000000000L) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
         return 0;
 
     if (0x0000000000000000L == un)
@@ -121,19 +175,92 @@ int main()
         return 0;
     if (0x0000000000000000L < un)
         return 0;
-    if (0x0000000000000000L <= un)
+    if (0x0000000000000000L <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
         return 0;
-    if (0x0000000000000000L > un)
+    if (0x0000000000000000L > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
         return 0;
     if (0x0000000000000000L >= un)
         return 0;
+
+    if (un == 0x0000000000000000UL)
+        return 0;
+    if (un != 0x0000000000000000UL)
+        return 0;
+    if (un < 0x0000000000000000UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+        return 0;
+    if (un <= 0x0000000000000000UL)
+        return 0;
+    if (un > 0x0000000000000000UL)
+        return 0;
+    if (un >= 0x0000000000000000UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
+        return 0;
+
+    if (0x0000000000000000UL == un)
+        return 0;
+    if (0x0000000000000000UL != un)
+        return 0;
+    if (0x0000000000000000UL < un)
+        return 0;
+    if (0x0000000000000000UL <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
+        return 0;
+    if (0x0000000000000000UL > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
+        return 0;
+    if (0x0000000000000000UL >= un)
+        return 0;
+
     float fl = 0;
-    if (fl == 0x0000000000000000L) // no warning
-      return 0;
+    if (fl == 0x0000000000000000L)
+        return 0;
+    if (fl != 0x0000000000000000L)
+        return 0;
+    if (fl < 0x0000000000000000L)
+        return 0;
+    if (fl <= 0x0000000000000000L)
+        return 0;
+    if (fl > 0x0000000000000000L)
+        return 0;
+    if (fl >= 0x0000000000000000L)
+        return 0;
 
-    float dl = 0;
-    if (dl == 0x0000000000000000L) // no warning
-      return 0;
+    if (0x0000000000000000L == fl)
+        return 0;
+    if (0x0000000000000000L != fl)
+        return 0;
+    if (0x0000000000000000L < fl)
+        return 0;
+    if (0x0000000000000000L <= fl)
+        return 0;
+    if (0x0000000000000000L > fl)
+        return 0;
+    if (0x0000000000000000L >= fl)
+        return 0;
+
+    double dl = 0;
+    if (dl == 0x0000000000000000L)
+        return 0;
+    if (dl != 0x0000000000000000L)
+        return 0;
+    if (dl < 0x0000000000000000L)
+        return 0;
+    if (dl <= 0x0000000000000000L)
+        return 0;
+    if (dl > 0x0000000000000000L)
+        return 0;
+    if (dl >= 0x0000000000000000L)
+        return 0;
+
+    if (0x0000000000000000L == dl)
+        return 0;
+    if (0x0000000000000000L != dl)
+        return 0;
+    if (0x0000000000000000L < dl)
+        return 0;
+    if (0x0000000000000000L <= dl)
+        return 0;
+    if (0x0000000000000000L > dl)
+        return 0;
+    if (0x0000000000000000L >= dl)
+        return 0;
 
     enum E {
     yes,

diff --git a/test/SemaCXX/compare.cpp b/test/SemaCXX/compare.cpp
index 0528b044fb1f1bdeba38e98ca0682ea3d2c331c2..1d940be629a0a1588f3f91026d59817a83933b80 100644 (file)
--- a/test/SemaCXX/compare.cpp
+++ b/test/SemaCXX/compare.cpp
@@ -73,7 +73,7 @@ int test0(long a, unsigned long b) {
          ((int) a == (unsigned int) B) +
          ((short) a == (unsigned short) B) +
          ((signed char) a == (unsigned char) B) +
-         (a < (unsigned long) B) +  // expected-warning {{comparison of integers of different signs}}
+         (a < (unsigned long) B) +  // expected-warning {{comparison of unsigned expression < 0 is always false}}
          (a < (unsigned int) B) +
          (a < (unsigned short) B) +
          (a < (unsigned char) B) +
@@ -81,8 +81,8 @@ int test0(long a, unsigned long b) {
          ((int) a < B) +
          ((short) a < B) +
          ((signed char) a < B) +
-         ((long) a < (unsigned long) B) +  // expected-warning {{comparison of integers of different signs}}
-         ((int) a < (unsigned int) B) +  // expected-warning {{comparison of integers of different signs}}
+         ((long) a < (unsigned long) B) +  // expected-warning {{comparison of unsigned expression < 0 is always false}}
+         ((int) a < (unsigned int) B) +  // expected-warning {{comparison of unsigned expression < 0 is always false}}
          ((short) a < (unsigned short) B) +
          ((signed char) a < (unsigned char) B) +