r372453 - Merge and improve code that detects same value in comparisons.
Richard Trieu via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 20 20:02:26 PDT 2019
Author: rtrieu
Date: Fri Sep 20 20:02:26 2019
New Revision: 372453
URL: http://llvm.org/viewvc/llvm-project?rev=372453&view=rev
Log:
Merge and improve code that detects same value in comparisons.
-Wtautological-overlap-compare and self-comparison from -Wtautological-compare
relay on detecting the same operand in different locations. Previously, each
warning had it's own operand checker. Now, both are merged together into
one function that each can call. The function also now looks through member
access and array accesses.
Differential Revision: https://reviews.llvm.org/D66045
Modified:
cfe/trunk/docs/ReleaseNotes.rst
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/lib/AST/Expr.cpp
cfe/trunk/lib/Analysis/CFG.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/Analysis/array-struct-region.cpp
cfe/trunk/test/Sema/warn-overlap.c
cfe/trunk/test/SemaCXX/compare-cxx2a.cpp
cfe/trunk/test/SemaCXX/self-comparison.cpp
Modified: cfe/trunk/docs/ReleaseNotes.rst
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=372453&r1=372452&r2=372453&view=diff
==============================================================================
--- cfe/trunk/docs/ReleaseNotes.rst (original)
+++ cfe/trunk/docs/ReleaseNotes.rst Fri Sep 20 20:02:26 2019
@@ -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
-------------------------------------------------
Modified: cfe/trunk/include/clang/AST/Expr.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=372453&r1=372452&r2=372453&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/Expr.h (original)
+++ cfe/trunk/include/clang/AST/Expr.h Fri Sep 20 20:02:26 2019
@@ -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;
Modified: cfe/trunk/lib/AST/Expr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Expr.cpp?rev=372453&r1=372452&r2=372453&view=diff
==============================================================================
--- cfe/trunk/lib/AST/Expr.cpp (original)
+++ cfe/trunk/lib/AST/Expr.cpp Fri Sep 20 20:02:26 2019
@@ -3921,6 +3921,111 @@ bool Expr::refersToGlobalRegisterVar() c
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 {
Modified: cfe/trunk/lib/Analysis/CFG.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=372453&r1=372452&r2=372453&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/CFG.cpp (original)
+++ cfe/trunk/lib/Analysis/CFG.cpp Fri Sep 20 20:02:26 2019
@@ -105,12 +105,12 @@ static const Expr *tryTransformToIntOrEn
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 BinaryO
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();
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=372453&r1=372452&r2=372453&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Sep 20 20:02:26 2019
@@ -10245,20 +10245,18 @@ static void diagnoseLogicalNotOnLHSofChe
<< 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 diagnoseTautologicalComparis
// 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 diagnoseTautologicalComparis
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 diagnoseTautologicalComparis
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) {
Modified: cfe/trunk/test/Analysis/array-struct-region.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/array-struct-region.cpp?rev=372453&r1=372452&r2=372453&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/array-struct-region.cpp (original)
+++ cfe/trunk/test/Analysis/array-struct-region.cpp Fri Sep 20 20:02:26 2019
@@ -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);
Modified: cfe/trunk/test/Sema/warn-overlap.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-overlap.c?rev=372453&r1=372452&r2=372453&view=diff
==============================================================================
--- cfe/trunk/test/Sema/warn-overlap.c (original)
+++ cfe/trunk/test/Sema/warn-overlap.c Fri Sep 20 20:02:26 2019
@@ -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 at -1{{overlapping comparisons always evaluate to false}}
+ return a.y == 1 || a.y != 1;
+ // expected-warning at -1{{overlapping comparisons always evaluate to true}}
+}
Modified: cfe/trunk/test/SemaCXX/compare-cxx2a.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/compare-cxx2a.cpp?rev=372453&r1=372452&r2=372453&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/compare-cxx2a.cpp (original)
+++ cfe/trunk/test/SemaCXX/compare-cxx2a.cpp Fri Sep 20 20:02:26 2019
@@ -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) {
Modified: cfe/trunk/test/SemaCXX/self-comparison.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/self-comparison.cpp?rev=372453&r1=372452&r2=372453&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/self-comparison.cpp (original)
+++ cfe/trunk/test/SemaCXX/self-comparison.cpp Fri Sep 20 20:02:26 2019
@@ -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
More information about the cfe-commits
mailing list