Index: clang-tidy/misc/RedundantExpressionCheck.cpp =================================================================== --- clang-tidy/misc/RedundantExpressionCheck.cpp +++ clang-tidy/misc/RedundantExpressionCheck.cpp @@ -98,6 +98,9 @@ case Stmt::StringLiteralClass: return cast(Left)->getBytes() == cast(Right)->getBytes(); + case Stmt::CXXOperatorCallExprClass: + return cast(Left)->getOperator() == + cast(Right)->getOperator(); case Stmt::DependentScopeDeclRefExprClass: if (cast(Left)->getDeclName() != cast(Right)->getDeclName()) @@ -410,6 +413,7 @@ std::string CastId = (Id + "-cast").str(); std::string SwapId = (Id + "-swap").str(); std::string NegateId = (Id + "-negate").str(); + std::string OverloadId = (Id + "-overload").str(); const auto RelationalExpr = ignoringParenImpCasts(binaryOperator( isComparisonOperator(), expr().bind(Id), @@ -437,12 +441,54 @@ hasOperatorName("!"), hasUnaryOperand(anyOf(CastExpr, RelationalExpr))))); + const auto OverloadedOperatorExpr = + cxxOperatorCallExpr( + anyOf(hasOverloadedOperatorName("=="), + hasOverloadedOperatorName("!="), hasOverloadedOperatorName("<"), + hasOverloadedOperatorName("<="), hasOverloadedOperatorName(">"), + hasOverloadedOperatorName(">=")), + // Filter noisy false positives. + unless(isMacro()), unless(isInTemplateInstantiation())) + .bind(OverloadId); + return anyOf(RelationalExpr, CastExpr, NegateRelationalExpr, - NegateNegateRelationalExpr); + NegateNegateRelationalExpr, OverloadedOperatorExpr); } -// Retrieves sub-expressions matched by 'matchRelationalIntegerConstantExpr' with -// name 'Id'. +// Checks whether a function param is non constant reference type, and may +// be modified in the function. +static bool isNonConstReferenceType(QualType ParamType) { + return ParamType->isReferenceType() && + !ParamType.getNonReferenceType().isConstQualified(); +} + +// Checks whether the arguments of an overloaded operator can be modified in the +// function. +// For operators that take an instance and a constant as arguments, only the +// first argument (the instance) needs to be checked, since the constant itself +// is a temporary expression. Whether the second parameter is checked is +// controlled by the parameter `ParamsToCheckCount`. +static bool +canOverloadedOperatorArgsBeModified(const FunctionDecl *OperatorDecl, + bool checkSecondParam) { + unsigned ParamCount = OperatorDecl->getNumParams(); + + // Overloaded operators declared inside a class have only one param. + // These functions must be declared const in order to not be able to modify + // the instance of the class they are called through. + if (ParamCount == 1 && + !OperatorDecl->getType()->getAs()->isConst()) + return true; + + if (isNonConstReferenceType(OperatorDecl->getParamDecl(0)->getType())) + return true; + + return checkSecondParam && ParamCount == 2 && + isNonConstReferenceType(OperatorDecl->getParamDecl(1)->getType()); +} + +// Retrieves sub-expressions matched by 'matchRelationalIntegerConstantExpr' +// with name 'Id'. static bool retrieveRelationalIntegerConstantExpr( const MatchFinder::MatchResult &Result, StringRef Id, const Expr *&OperandExpr, BinaryOperatorKind &Opcode, const Expr *&Symbol, @@ -450,6 +496,7 @@ std::string CastId = (Id + "-cast").str(); std::string SwapId = (Id + "-swap").str(); std::string NegateId = (Id + "-negate").str(); + std::string OverloadId = (Id + "-overload").str(); if (const auto *Bin = Result.Nodes.getNodeAs(Id)) { // Operand received with explicit comparator. @@ -458,12 +505,29 @@ if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr)) return false; - } else if (const auto *Cast = Result.Nodes.getNodeAs(CastId)) { // Operand received with implicit comparator (cast). Opcode = BO_NE; OperandExpr = Cast; Value = APSInt(32, false); + } else if (const auto *OverloadedOperatorExpr = + Result.Nodes.getNodeAs(OverloadId)) { + const auto *OverloadedFunctionDecl = dyn_cast_or_null(OverloadedOperatorExpr->getCalleeDecl()); + if (!OverloadedFunctionDecl) + return false; + + if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, false)) + return false; + + if (!OverloadedOperatorExpr->getArg(1)->isIntegerConstantExpr( + Value, *Result.Context)) + return false; + + Symbol = OverloadedOperatorExpr->getArg(0); + OperandExpr = OverloadedOperatorExpr; + Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator()); + + return BinaryOperator::isComparisonOp(Opcode); } else { return false; } @@ -548,7 +612,8 @@ Lexer::getImmediateMacroName(RhsLoc, SM, LO)); } -static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, const Expr *&RhsExpr) { +static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, + const Expr *&RhsExpr) { if (!LhsExpr || !RhsExpr) return false; @@ -562,7 +627,8 @@ const auto AnyLiteralExpr = ignoringParenImpCasts( anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral())); - const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(KnownBannedMacroNames)); + const auto BannedIntegerLiteral = + integerLiteral(expandedByMacro(KnownBannedMacroNames)); // Binary with equivalent operands, like (X != 2 && X != 2). Finder->addMatcher( @@ -584,13 +650,12 @@ this); // Conditional (trenary) operator with equivalent operands, like (Y ? X : X). - Finder->addMatcher( - conditionalOperator(expressionsAreEquivalent(), - // Filter noisy false positives. - unless(conditionalOperatorIsInMacro()), - unless(isInTemplateInstantiation())) - .bind("cond"), - this); + Finder->addMatcher(conditionalOperator(expressionsAreEquivalent(), + // Filter noisy false positives. + unless(conditionalOperatorIsInMacro()), + unless(isInTemplateInstantiation())) + .bind("cond"), + this); // Overloaded operators with equivalent operands. Finder->addMatcher( @@ -821,16 +886,15 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *BinOp = Result.Nodes.getNodeAs("binary")) { - // If the expression's constants are macros, check whether they are // intentional. if (areSidesBinaryConstExpressions(BinOp, Result.Context)) { const Expr *LhsConst = nullptr, *RhsConst = nullptr; BinaryOperatorKind MainOpcode, SideOpcode; - if(!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode, LhsConst, - RhsConst, Result.Context)) - return; + if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode, + LhsConst, RhsConst, Result.Context)) + return; if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) || areExprsMacroAndNonMacro(LhsConst, RhsConst)) @@ -853,6 +917,13 @@ } if (const auto *Call = Result.Nodes.getNodeAs("call")) { + const auto *OverloadedFunctionDecl = dyn_cast_or_null(Call->getCalleeDecl()); + if (!OverloadedFunctionDecl) + return; + + if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, true)) + return; + diag(Call->getOperatorLoc(), "both sides of overloaded operator are equivalent"); } Index: test/clang-tidy/misc-redundant-expression.cpp =================================================================== --- test/clang-tidy/misc-redundant-expression.cpp +++ test/clang-tidy/misc-redundant-expression.cpp @@ -124,16 +124,36 @@ #undef COND_OP_MACRO #undef COND_OP_OTHER_MACRO +// Overloaded operators that compare two instances of a struct. struct MyStruct { - int x; + int x; + bool operator==(const MyStruct& rhs) const {return this->x == rhs.x; } // not modifing + bool operator>=(const MyStruct& rhs) const { return this->x >= rhs.x; } // not modifing + bool operator<=(MyStruct& rhs) const { return this->x <= rhs.x; } + bool operator&&(const MyStruct& rhs){ this->x++; return this->x && rhs.x; } } Q; -bool operator==(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; } +bool operator!=(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; } // not modifing +bool operator<(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x < rhs.x; } // not modifing +bool operator>(const MyStruct& lhs, MyStruct& rhs) { rhs.x--; return lhs.x > rhs.x; } +bool operator||(MyStruct& lhs, const MyStruct& rhs) { lhs.x++; return lhs.x || rhs.x; } -bool TestOperator(MyStruct& S) { +bool TestOverloadedOperator(MyStruct& S) { if (S == Q) return false; + + if (S <= S) return false; + if (S && S) return false; + if (S > S) return false; + if (S || S) return false; + if (S == S) return true; // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent + if (S < S) return true; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent + if (S != S) return true; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent + if (S >= S) return true; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent } #define LT(x, y) (void)((x) < (y)) @@ -176,7 +196,7 @@ void TemplateCheck() { static_assert(T::Value == U::Value, "should be identical"); static_assert(T::Value == T::Value, "should be identical"); - // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of overloaded operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent } void TestTemplate() { TemplateCheck(); } @@ -281,9 +301,46 @@ return 0; } +// Overloaded operators that compare an instance of a struct and an integer +// constant. +struct S { + S() { x = 1; } + int x; + // Overloaded comparison operators without any possible side effect. + bool operator==(const int &i) const { return x == i; } // not modifying + bool operator!=(int i) const { return x != i; } // not modifying + bool operator>(const int &i) const { return x > i; } // not modifying + bool operator<(int i) const { return x < i; } // not modifying +}; + +bool operator<=(const S &s, int i) { return s.x <= i; } // not modifying +bool operator>=(const S &s, const int &i) { return s.x >= i; } // not modifying + +struct S2 { + S2() { x = 1; } + int x; + // Overloaded comparison operators that are able to modify their params. + bool operator==(const int &i) { + this->x++; + return x == i; + } + bool operator!=(int i) { return x != i; } + bool operator>(const int &i) { return x > i; } + bool operator<(int i) { + this->x--; + return x < i; + } +}; + +bool operator>=(S2 &s, const int &i) { return s.x >= i; } +bool operator<=(S2 &s, int i) { + s.x++; + return s.x <= i; +} + int TestLogical(int X, int Y){ #define CONFIG 0 - if (CONFIG && X) return 1; // OK, consts from macros are considered intentional + if (CONFIG && X) return 1; #undef CONFIG #define CONFIG 1 if (CONFIG || X) return 1; @@ -331,6 +388,24 @@ if (!X && Y) return 1; if (!X && Y == 0) return 1; if (X == 10 && Y != 10) return 1; + + // Test for overloaded operators with constant params. + S s1; + if (s1 == 1 && s1 == 1) return true; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: equivalent expression on both sides of logical operator + if (s1 == 1 || s1 != 1) return true; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true + if (s1 > 1 && s1 < 1) return true; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false + if (s1 >= 1 || s1 <= 1) return true; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true + + // Test for overloaded operators that may modify their params. + S2 s2; + if (s2 == 1 || s2 != 1) return true; + if (s2 == 1 || s2 == 1) return true; + if (s2 > 1 && s2 < 1) return true; + if (s2 >= 1 || s2 <= 1) return true; } int TestRelational(int X, int Y) {