Index: include/clang/AST/Expr.h =================================================================== --- include/clang/AST/Expr.h +++ include/clang/AST/Expr.h @@ -906,6 +906,11 @@ 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; Index: lib/AST/Expr.cpp =================================================================== --- lib/AST/Expr.cpp +++ lib/AST/Expr.cpp @@ -3915,6 +3915,90 @@ 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(E1); + const auto *DRE2 = cast(E2); + return DRE1->isRValue() && DRE2->isRValue() && + DRE1->getDecl() == DRE2->getDecl(); + } + case ImplicitCastExprClass: { + // Peel off implicit casts. + while (true) { + const auto *ICE1 = dyn_cast(E1); + const auto *ICE2 = dyn_cast(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(E1); + const auto *DRE2 = dyn_cast(E2); + if (DRE1 && DRE2) + return declaresSameEntity(DRE1->getDecl(), DRE2->getDecl()); + + const auto *Ivar1 = dyn_cast(E1); + const auto *Ivar2 = dyn_cast(E2); + if (Ivar1 && Ivar2) { + return Ivar1->isFreeIvar() && Ivar2->isFreeIvar() && + declaresSameEntity(Ivar1->getDecl(), Ivar2->getDecl()); + } + + // Walk the MemberExpr chain. + while (isa(E1) && isa(E2)) { + const auto *ME1 = cast(E1); + const auto *ME2 = cast(E2); + if (!declaresSameEntity(ME1->getMemberDecl(), ME2->getMemberDecl())) + return false; + if (const auto *D = dyn_cast(ME1->getMemberDecl())) + if (D->isStaticDataMember()) + return true; + E1 = ME1->getBase()->IgnoreParens(); + E2 = ME2->getBase()->IgnoreParens(); + } + + if (isa(E1) && isa(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 (auto *DRE = dyn_cast(E)) + return DRE->getDecl(); + if (auto *ME = dyn_cast(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 { Index: lib/Analysis/CFG.cpp =================================================================== --- lib/Analysis/CFG.cpp +++ lib/Analysis/CFG.cpp @@ -81,12 +81,12 @@ 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 +static std::tuple tryNormalizeBinaryOperator(const BinaryOperator *B) { BinaryOperatorKind Op = B->getOpcode(); @@ -108,8 +108,7 @@ Constant = tryTransformToIntOrEnumConstant(B->getLHS()); } - auto *D = dyn_cast(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 @@ -1017,34 +1016,34 @@ 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(); Index: lib/Sema/SemaExpr.cpp =================================================================== --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -10126,20 +10126,18 @@ << 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(E)) - return DR->getDecl(); - if (ObjCIvarRefExpr *Ivar = dyn_cast(E)) { - if (Ivar->isFreeIvar()) - return Ivar->getDecl(); - } - if (MemberExpr *Mem = dyn_cast(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(E)) { + D = DR->getDecl(); + } else if (const MemberExpr *Mem = dyn_cast(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. @@ -10172,8 +10170,6 @@ // 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 { @@ -10182,7 +10178,8 @@ 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: @@ -10202,9 +10199,7 @@ 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) { Index: test/Analysis/array-struct-region.cpp =================================================================== --- test/Analysis/array-struct-region.cpp +++ test/Analysis/array-struct-region.cpp @@ -26,8 +26,9 @@ const struct S *getThis() const { return this; } const struct S *operator +() const { return this; } - bool check() const { return this == this; } - bool operator !() const { return this != this; } + // Adding zero to the this pointer silences a tautological compare warning. + bool check() const { return this == this + 0; } + bool operator !() const { return this != this + 0; } int operator *() const { return field; } #endif Index: test/Sema/warn-overlap.c =================================================================== --- test/Sema/warn-overlap.c +++ test/Sema/warn-overlap.c @@ -141,3 +141,17 @@ return x < 1 || x != 0; // expected-warning@-1{{overlapping comparisons always evaluate to true}} } + +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}} +} Index: test/SemaCXX/self-comparison.cpp =================================================================== --- test/SemaCXX/self-comparison.cpp +++ test/SemaCXX/self-comparison.cpp @@ -40,3 +40,48 @@ Y::n == Y::n; } template bool g(); // 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; + } +}; + +struct S { + int field; + static int static_field; + int array[1]; +}; + +struct T { + int field; + static int static_field; + int array[1]; + S s; +}; + +int struct_test(S s1, S s2, 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}} + + // no warning + return s1.field == s2.field; + return s2.array == s1.array; + + return s1.static_field == t.static_field; +}; +} // namespace member_tests