summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--docs/ReleaseNotes.rst3
-rw-r--r--include/clang/AST/Expr.h5
-rw-r--r--lib/AST/Expr.cpp105
-rw-r--r--lib/Analysis/CFG.cpp33
-rw-r--r--lib/Sema/SemaExpr.cpp31
-rw-r--r--test/Analysis/array-struct-region.cpp6
-rw-r--r--test/Sema/warn-overlap.c14
-rw-r--r--test/SemaCXX/compare-cxx2a.cpp6
-rw-r--r--test/SemaCXX/self-comparison.cpp81
9 files changed, 249 insertions, 35 deletions
diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst
index e04e568ddb..bb93fb5477 100644
--- a/docs/ReleaseNotes.rst
+++ b/docs/ReleaseNotes.rst
@@ -53,6 +53,9 @@ Improvements to Clang's diagnostics
- -Wtautological-overlap-compare will warn on negative numbers and non-int
types.
+- -Wtautological-compare for self comparisons and
+ -Wtautological-overlap-compare will now look through member and array
+ access to determine if two operand expressions are the same.
Non-comprehensive list of changes in this release
-------------------------------------------------
diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h
index 30fa84315f..ffa7d4db96 100644
--- a/include/clang/AST/Expr.h
+++ b/include/clang/AST/Expr.h
@@ -906,6 +906,11 @@ public:
return skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
}
+ /// Checks that the two Expr's will refer to the same value as a comparison
+ /// operand. The caller must ensure that the values referenced by the Expr's
+ /// are not modified between E1 and E2 or the result my be invalid.
+ static bool isSameComparisonOperand(const Expr* E1, const Expr* E2);
+
static bool classof(const Stmt *T) {
return T->getStmtClass() >= firstExprConstant &&
T->getStmtClass() <= lastExprConstant;
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
index 00c6a81704..b5fb7fafb0 100644
--- a/lib/AST/Expr.cpp
+++ b/lib/AST/Expr.cpp
@@ -3921,6 +3921,111 @@ bool Expr::refersToGlobalRegisterVar() const {
return false;
}
+bool Expr::isSameComparisonOperand(const Expr* E1, const Expr* E2) {
+ E1 = E1->IgnoreParens();
+ E2 = E2->IgnoreParens();
+
+ if (E1->getStmtClass() != E2->getStmtClass())
+ return false;
+
+ switch (E1->getStmtClass()) {
+ default:
+ return false;
+ case CXXThisExprClass:
+ return true;
+ case DeclRefExprClass: {
+ // DeclRefExpr without an ImplicitCastExpr can happen for integral
+ // template parameters.
+ const auto *DRE1 = cast<DeclRefExpr>(E1);
+ const auto *DRE2 = cast<DeclRefExpr>(E2);
+ return DRE1->isRValue() && DRE2->isRValue() &&
+ DRE1->getDecl() == DRE2->getDecl();
+ }
+ case ImplicitCastExprClass: {
+ // Peel off implicit casts.
+ while (true) {
+ const auto *ICE1 = dyn_cast<ImplicitCastExpr>(E1);
+ const auto *ICE2 = dyn_cast<ImplicitCastExpr>(E2);
+ if (!ICE1 || !ICE2)
+ return false;
+ if (ICE1->getCastKind() != ICE2->getCastKind())
+ return false;
+ E1 = ICE1->getSubExpr()->IgnoreParens();
+ E2 = ICE2->getSubExpr()->IgnoreParens();
+ // The final cast must be one of these types.
+ if (ICE1->getCastKind() == CK_LValueToRValue ||
+ ICE1->getCastKind() == CK_ArrayToPointerDecay ||
+ ICE1->getCastKind() == CK_FunctionToPointerDecay) {
+ break;
+ }
+ }
+
+ const auto *DRE1 = dyn_cast<DeclRefExpr>(E1);
+ const auto *DRE2 = dyn_cast<DeclRefExpr>(E2);
+ if (DRE1 && DRE2)
+ return declaresSameEntity(DRE1->getDecl(), DRE2->getDecl());
+
+ const auto *Ivar1 = dyn_cast<ObjCIvarRefExpr>(E1);
+ const auto *Ivar2 = dyn_cast<ObjCIvarRefExpr>(E2);
+ if (Ivar1 && Ivar2) {
+ return Ivar1->isFreeIvar() && Ivar2->isFreeIvar() &&
+ declaresSameEntity(Ivar1->getDecl(), Ivar2->getDecl());
+ }
+
+ const auto *Array1 = dyn_cast<ArraySubscriptExpr>(E1);
+ const auto *Array2 = dyn_cast<ArraySubscriptExpr>(E2);
+ if (Array1 && Array2) {
+ if (!isSameComparisonOperand(Array1->getBase(), Array2->getBase()))
+ return false;
+
+ auto Idx1 = Array1->getIdx();
+ auto Idx2 = Array2->getIdx();
+ const auto Integer1 = dyn_cast<IntegerLiteral>(Idx1);
+ const auto Integer2 = dyn_cast<IntegerLiteral>(Idx2);
+ if (Integer1 && Integer2) {
+ if (Integer1->getValue() != Integer2->getValue())
+ return false;
+ } else {
+ if (!isSameComparisonOperand(Idx1, Idx2))
+ return false;
+ }
+
+ return true;
+ }
+
+ // Walk the MemberExpr chain.
+ while (isa<MemberExpr>(E1) && isa<MemberExpr>(E2)) {
+ const auto *ME1 = cast<MemberExpr>(E1);
+ const auto *ME2 = cast<MemberExpr>(E2);
+ if (!declaresSameEntity(ME1->getMemberDecl(), ME2->getMemberDecl()))
+ return false;
+ if (const auto *D = dyn_cast<VarDecl>(ME1->getMemberDecl()))
+ if (D->isStaticDataMember())
+ return true;
+ E1 = ME1->getBase()->IgnoreParenImpCasts();
+ E2 = ME2->getBase()->IgnoreParenImpCasts();
+ }
+
+ if (isa<CXXThisExpr>(E1) && isa<CXXThisExpr>(E2))
+ return true;
+
+ // A static member variable can end the MemberExpr chain with either
+ // a MemberExpr or a DeclRefExpr.
+ auto getAnyDecl = [](const Expr *E) -> const ValueDecl * {
+ if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
+ return DRE->getDecl();
+ if (const auto *ME = dyn_cast<MemberExpr>(E))
+ return ME->getMemberDecl();
+ return nullptr;
+ };
+
+ const ValueDecl *VD1 = getAnyDecl(E1);
+ const ValueDecl *VD2 = getAnyDecl(E2);
+ return declaresSameEntity(VD1, VD2);
+ }
+ }
+}
+
/// isArrow - Return true if the base expression is a pointer to vector,
/// return false if the base expression is a vector.
bool ExtVectorElementExpr::isArrow() const {
diff --git a/lib/Analysis/CFG.cpp b/lib/Analysis/CFG.cpp
index 86816997f0..687cf57059 100644
--- a/lib/Analysis/CFG.cpp
+++ b/lib/Analysis/CFG.cpp
@@ -105,12 +105,12 @@ static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
return nullptr;
}
-/// Tries to interpret a binary operator into `Decl Op Expr` form, if Expr is
-/// an integer literal or an enum constant.
+/// Tries to interpret a binary operator into `Expr Op NumExpr` form, if
+/// NumExpr is an integer literal or an enum constant.
///
/// If this fails, at least one of the returned DeclRefExpr or Expr will be
/// null.
-static std::tuple<const DeclRefExpr *, BinaryOperatorKind, const Expr *>
+static std::tuple<const Expr *, BinaryOperatorKind, const Expr *>
tryNormalizeBinaryOperator(const BinaryOperator *B) {
BinaryOperatorKind Op = B->getOpcode();
@@ -132,8 +132,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) {
Constant = tryTransformToIntOrEnumConstant(B->getLHS());
}
- auto *D = dyn_cast<DeclRefExpr>(MaybeDecl->IgnoreParenImpCasts());
- return std::make_tuple(D, Op, Constant);
+ return std::make_tuple(MaybeDecl, Op, Constant);
}
/// For an expression `x == Foo && x == Bar`, this determines whether the
@@ -1045,34 +1044,34 @@ private:
if (!LHS->isComparisonOp() || !RHS->isComparisonOp())
return {};
- const DeclRefExpr *Decl1;
- const Expr *Expr1;
+ const Expr *DeclExpr1;
+ const Expr *NumExpr1;
BinaryOperatorKind BO1;
- std::tie(Decl1, BO1, Expr1) = tryNormalizeBinaryOperator(LHS);
+ std::tie(DeclExpr1, BO1, NumExpr1) = tryNormalizeBinaryOperator(LHS);
- if (!Decl1 || !Expr1)
+ if (!DeclExpr1 || !NumExpr1)
return {};
- const DeclRefExpr *Decl2;
- const Expr *Expr2;
+ const Expr *DeclExpr2;
+ const Expr *NumExpr2;
BinaryOperatorKind BO2;
- std::tie(Decl2, BO2, Expr2) = tryNormalizeBinaryOperator(RHS);
+ std::tie(DeclExpr2, BO2, NumExpr2) = tryNormalizeBinaryOperator(RHS);
- if (!Decl2 || !Expr2)
+ if (!DeclExpr2 || !NumExpr2)
return {};
// Check that it is the same variable on both sides.
- if (Decl1->getDecl() != Decl2->getDecl())
+ if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2))
return {};
// Make sure the user's intent is clear (e.g. they're comparing against two
// int literals, or two things from the same enum)
- if (!areExprTypesCompatible(Expr1, Expr2))
+ if (!areExprTypesCompatible(NumExpr1, NumExpr2))
return {};
Expr::EvalResult L1Result, L2Result;
- if (!Expr1->EvaluateAsInt(L1Result, *Context) ||
- !Expr2->EvaluateAsInt(L2Result, *Context))
+ if (!NumExpr1->EvaluateAsInt(L1Result, *Context) ||
+ !NumExpr2->EvaluateAsInt(L2Result, *Context))
return {};
llvm::APSInt L1 = L1Result.Val.getInt();
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 808c0e450a..6dfbc4cda5 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -10245,20 +10245,18 @@ static void diagnoseLogicalNotOnLHSofCheck(Sema &S, ExprResult &LHS,
<< FixItHint::CreateInsertion(SecondClose, ")");
}
-// Get the decl for a simple expression: a reference to a variable,
-// an implicit C++ field reference, or an implicit ObjC ivar reference.
-static ValueDecl *getCompareDecl(Expr *E) {
- if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
- return DR->getDecl();
- if (ObjCIvarRefExpr *Ivar = dyn_cast<ObjCIvarRefExpr>(E)) {
- if (Ivar->isFreeIvar())
- return Ivar->getDecl();
- }
- if (MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
+// Returns true if E refers to a non-weak array.
+static bool checkForArray(const Expr *E) {
+ const ValueDecl *D = nullptr;
+ if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E)) {
+ D = DR->getDecl();
+ } else if (const MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
if (Mem->isImplicitAccess())
- return Mem->getMemberDecl();
+ D = Mem->getMemberDecl();
}
- return nullptr;
+ if (!D)
+ return false;
+ return D->getType()->isArrayType() && !D->isWeak();
}
/// Diagnose some forms of syntactically-obvious tautological comparison.
@@ -10291,8 +10289,6 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
// obvious cases in the definition of the template anyways. The idea is to
// warn when the typed comparison operator will always evaluate to the same
// result.
- ValueDecl *DL = getCompareDecl(LHSStripped);
- ValueDecl *DR = getCompareDecl(RHSStripped);
// Used for indexing into %select in warn_comparison_always
enum {
@@ -10301,7 +10297,8 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
AlwaysFalse,
AlwaysEqual, // std::strong_ordering::equal from operator<=>
};
- if (DL && DR && declaresSameEntity(DL, DR)) {
+
+ if (Expr::isSameComparisonOperand(LHS, RHS)) {
unsigned Result;
switch (Opc) {
case BO_EQ: case BO_LE: case BO_GE:
@@ -10321,9 +10318,7 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
S.PDiag(diag::warn_comparison_always)
<< 0 /*self-comparison*/
<< Result);
- } else if (DL && DR &&
- DL->getType()->isArrayType() && DR->getType()->isArrayType() &&
- !DL->isWeak() && !DR->isWeak()) {
+ } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
// What is it always going to evaluate to?
unsigned Result;
switch(Opc) {
diff --git a/test/Analysis/array-struct-region.cpp b/test/Analysis/array-struct-region.cpp
index cfb57d3924..1b9fa3e8db 100644
--- a/test/Analysis/array-struct-region.cpp
+++ b/test/Analysis/array-struct-region.cpp
@@ -1,20 +1,26 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -x c %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -x c++ -std=c++14 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -x c++ -std=c++17 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -DINLINE -x c %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -DINLINE -x c++ -std=c++14 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -DINLINE -x c++ -std=c++17 %s
void clang_analyzer_eval(int);
diff --git a/test/Sema/warn-overlap.c b/test/Sema/warn-overlap.c
index d72e60755d..066312591c 100644
--- a/test/Sema/warn-overlap.c
+++ b/test/Sema/warn-overlap.c
@@ -158,3 +158,17 @@ int no_warning(unsigned x) {
return x >= 0 || x == 1;
// no warning since "x >= 0" is caught by a different tautological warning.
}
+
+struct A {
+ int x;
+ int y;
+};
+
+int struct_test(struct A a) {
+ return a.x > 5 && a.y < 1; // no warning, different variables
+
+ return a.x > 5 && a.x < 1;
+ // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ return a.y == 1 || a.y != 1;
+ // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
diff --git a/test/SemaCXX/compare-cxx2a.cpp b/test/SemaCXX/compare-cxx2a.cpp
index c68a0ae913..b6e7fc8061 100644
--- a/test/SemaCXX/compare-cxx2a.cpp
+++ b/test/SemaCXX/compare-cxx2a.cpp
@@ -8,12 +8,18 @@
#define ASSERT_TYPE(...) static_assert(__is_same(__VA_ARGS__))
#define ASSERT_EXPR_TYPE(Expr, Expect) static_assert(__is_same(decltype(Expr), Expect));
+struct S {
+ static int x[5];
+};
+
void self_compare() {
int a;
int *b = nullptr;
+ S s;
(void)(a <=> a); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
(void)(b <=> b); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
+ (void)(s.x[a] <=> S::x[a]); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
}
void test0(long a, unsigned long b) {
diff --git a/test/SemaCXX/self-comparison.cpp b/test/SemaCXX/self-comparison.cpp
index ac129b68a6..4ea16665f1 100644
--- a/test/SemaCXX/self-comparison.cpp
+++ b/test/SemaCXX/self-comparison.cpp
@@ -40,3 +40,84 @@ bool g() {
Y<T>::n == Y<U>::n;
}
template bool g<int, int>(); // should not produce any warnings
+
+namespace member_tests {
+struct B {
+ int field;
+ static int static_field;
+ int test(B b) {
+ return field == field; // expected-warning {{self-comparison always evaluates to true}}
+ return static_field == static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return static_field == b.static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return B::static_field == this->static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return this == this; // expected-warning {{self-comparison always evaluates to true}}
+
+ return field == b.field;
+ return this->field == b.field;
+ }
+};
+
+enum {
+ I0,
+ I1,
+ I2,
+};
+
+struct S {
+ int field;
+ static int static_field;
+ int array[4];
+};
+
+struct T {
+ int field;
+ static int static_field;
+ int array[4];
+ S s;
+};
+
+int struct_test(S s1, S s2, S *s3, T t) {
+ return s1.field == s1.field; // expected-warning {{self-comparison always evaluates to true}}
+ return s2.field == s2.field; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.static_field == s2.static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return S::static_field == s1.static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.array == s1.array; // expected-warning {{self-comparison always evaluates to true}}
+ return t.s.static_field == S::static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return s3->field == s3->field; // expected-warning {{self-comparison always evaluates to true}}
+ return s3->static_field == S::static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.array[0] == s1.array[0]; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.array[I1] == s1.array[I1]; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.array[s2.array[0]] == s1.array[s2.array[0]]; // expected-warning {{self-comparison always evaluates to true}}
+ return s3->array[t.field] == s3->array[t.field]; // expected-warning {{self-comparison always evaluates to true}}
+
+ // Try all operators
+ return t.field == t.field; // expected-warning {{self-comparison always evaluates to true}}
+ return t.field <= t.field; // expected-warning {{self-comparison always evaluates to true}}
+ return t.field >= t.field; // expected-warning {{self-comparison always evaluates to true}}
+
+ return t.field != t.field; // expected-warning {{self-comparison always evaluates to false}}
+ return t.field < t.field; // expected-warning {{self-comparison always evaluates to false}}
+ return t.field > t.field; // expected-warning {{self-comparison always evaluates to false}}
+
+ // no warning
+ return s1.field == s2.field;
+ return s2.array == s1.array;
+ return s2.array[0] == s1.array[0];
+ return s1.array[I1] == s1.array[I2];
+
+ return s1.static_field == t.static_field;
+};
+
+struct U {
+ bool operator!=(const U&);
+};
+
+bool operator==(const U&, const U&);
+
+// May want to warn on this in the future.
+int user_defined(U u) {
+ return u == u;
+ return u != u;
+}
+
+} // namespace member_tests
View Lorry Controller Status