[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: [cfe-commits] [PATCH] Enhance -Wtautological-compare
From: Xi Wang <xi.wang () gmail ! com>
Date: 2012-02-29 16:36:05
Message-ID: C3DEC37F-779C-4C3F-B503-A52D691A9509 () gmail ! com
[Download RAW message or body]
Clang doesn't issue warnings against tautological comparisons like
(uchar < 0) and (uchar == -1). Such tautological comparisons are
often logic bugs that break the error handling. Below are some
recent examples.
http://git.kernel.org/linus/3a7f8fb1
http://git.kernel.org/linus/589665f5
http://git.kernel.org/linus/4690c33d
This patch enhances Clang to catch such tautological comparisons,
where bool/uchar/ushort is sign-extended to signed int. Clang
currently only warns against unsigned tautological comparisons.
- xi
["scmp.patch" (scmp.patch)]
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index ec4604a..b3c0ae5 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3641,6 +3641,9 @@ def warn_lunsigned_always_true_comparison : Warning<
def warn_runsigned_always_true_comparison : Warning<
"comparison of %0 unsigned%select{| enum}2 expression is always %1">,
InGroup<TautologicalCompare>;
+def warn_always_true_comparison : Warning<
+ "comparison of %0 %1 %2 is always %select{false|true}3">,
+ InGroup<TautologicalCompare>;
def warn_comparison_of_mixed_enum_types : Warning<
"comparison of two values with different enumeration types (%0 and %1)">,
InGroup<DiagGroup<"enum-compare">>;
diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index e963065..025b855 100644
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -3664,7 +3664,7 @@ static bool IsSameFloatAfterCast(const APValue &value,
static void AnalyzeImplicitConversions(Sema &S, Expr *E, SourceLocation CC);
-static bool IsZero(Sema &S, Expr *E) {
+static bool IsIntegerConstant(llvm::APSInt &Value, Sema &S, Expr *E) {
// Suppress cases where we are comparing against an enum constant.
if (const DeclRefExpr *DR =
dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
@@ -3675,8 +3675,16 @@ static bool IsZero(Sema &S, Expr *E) {
if (E->getLocStart().isMacroID())
return false;
+ return E->isIntegerConstantExpr(Value, S.Context);
+}
+
+static bool IsNegative(llvm::APSInt &Value, Sema &S, Expr *E) {
+ return IsIntegerConstant(Value, S, E) && Value.isNegative();
+}
+
+static bool IsZero(Sema &S, Expr *E) {
llvm::APSInt Value;
- return E->isIntegerConstantExpr(Value, S.Context) && Value == 0;
+ return IsIntegerConstant(Value, S, E) && Value == 0;
}
static bool HasEnumType(Expr *E) {
@@ -3691,6 +3699,47 @@ static bool HasEnumType(Expr *E) {
return E->getType()->isEnumeralType();
}
+static bool IsUnsigned(Expr *E) {
+ return E->getType()->isUnsignedIntegerType();
+}
+
+static void CheckTrivialSignedComparison(Sema &S, BinaryOperator *E) {
+ BinaryOperatorKind Op = E->getOpcode();
+ const char *OpStr = E->getOpcodeStr();
+ Expr *LHS = E->getLHS()->IgnoreParenImpCasts();
+ Expr *RHS = E->getRHS()->IgnoreParenImpCasts();
+ // We are only interested in 'unsigned cmp signed-constant'. Since
+ // the comparison is signed, the unsigned part must be sign-extended.
+ if (E->isEqualityOp()) {
+ llvm::APSInt Value;
+ if (IsUnsigned(LHS) && IsNegative(Value, S, RHS)) {
+ S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
+ << LHS->getType() << OpStr << Value.toString(10) << (Op == BO_NE)
+ << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+ } else if (IsNegative(Value, S, LHS) && IsUnsigned(RHS)) {
+ S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
+ << Value.toString(10) << OpStr << RHS->getType() << (Op == BO_NE)
+ << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+ }
+ } else if (Op == BO_LT && IsUnsigned(LHS) && IsZero(S, RHS)) {
+ S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
+ << LHS->getType() << OpStr << "0" << 0
+ << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+ } else if (Op == BO_GE && IsUnsigned(LHS) && IsZero(S, RHS)) {
+ S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
+ << LHS->getType() << OpStr << "0" << 1
+ << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+ } else if (Op == BO_GT && IsZero(S, LHS) && IsUnsigned(RHS)) {
+ S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
+ << "0" << OpStr << RHS->getType() << 0
+ << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+ } else if (Op == BO_LE && IsZero(S, LHS) && IsUnsigned(RHS)) {
+ S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
+ << "0" << OpStr << RHS->getType() << 1
+ << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+ }
+}
+
static void CheckTrivialUnsignedComparison(Sema &S, BinaryOperator *E) {
BinaryOperatorKind op = E->getOpcode();
if (E->isValueDependent())
@@ -3731,14 +3780,19 @@ static void AnalyzeComparison(Sema &S, BinaryOperator *E) {
assert(S.Context.hasSameUnqualifiedType(T, E->getRHS()->getType())
&& "comparison with mismatched types");
+ // We don't care about value-dependent expressions or expressions
+ // whose result is a constant.
+ if (E->isValueDependent() || E->isIntegerConstantExpr(S.Context))
+ return AnalyzeImpConvsInComparison(S, E);
+
+ // Check trivial signed comparisons.
+ if (T->hasSignedIntegerRepresentation())
+ CheckTrivialSignedComparison(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()
- || E->isValueDependent() || E->isIntegerConstantExpr(S.Context))
+ if (!T->hasUnsignedIntegerRepresentation())
return AnalyzeImpConvsInComparison(S, E);
Expr *LHS = E->getLHS()->IgnoreParenImpCasts();
diff --git a/test/Sema/compare.c b/test/Sema/compare.c
index 03aebb3..f340fea 100644
--- a/test/Sema/compare.c
+++ b/test/Sema/compare.c
@@ -284,6 +284,19 @@ int test5(unsigned int x) {
&& (0 <= x); // expected-warning {{comparison of 0 <= unsigned expression is always true}}
}
+int uchar(unsigned char x) {
+ return 42
+ + (x < 0) // expected-warning {{comparison of 'unsigned char' < 0 is always false}}
+ + (0 > x) // expected-warning {{comparison of 0 > 'unsigned char' is always false}}
+ + (x >= 0) // expected-warning {{comparison of 'unsigned char' >= 0 is always true}}
+ + (0 <= x) // expected-warning {{comparison of 0 <= 'unsigned char' is always true}}
+ + (x == -1) // expected-warning {{comparison of 'unsigned char' == -1 is always false}}
+ + (-1 == x) // expected-warning {{comparison of -1 == 'unsigned char' is always false}}
+ + (x != -1) // expected-warning {{comparison of 'unsigned char' != -1 is always true}}
+ + (-1 != x) // expected-warning {{comparison of -1 != 'unsigned char' is always true}}
+ ;
+}
+
int test6(unsigned i, unsigned power) {
unsigned x = (i < (1 << power) ? i : 0);
return x != 3 ? 1 << power : i;
diff --git a/test/SemaTemplate/instantiate-function-1.cpp b/test/SemaTemplate/instantiate-function-1.cpp
index ceef274..3508d88 100644
--- a/test/SemaTemplate/instantiate-function-1.cpp
+++ b/test/SemaTemplate/instantiate-function-1.cpp
@@ -65,7 +65,7 @@ template<typename T, typename U, typename V> struct X6 {
if (t > 0)
return u;
else {
- if (t < 0)
+ if (t < 0) // expected-warning{{comparison of 'bool' < 0 is always false}}
return v; // expected-error{{cannot initialize return object of type}}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic