Index: clang-tidy/misc/RedundantExpressionCheck.cpp =================================================================== --- clang-tidy/misc/RedundantExpressionCheck.cpp +++ clang-tidy/misc/RedundantExpressionCheck.cpp @@ -38,7 +38,7 @@ } // namespace static const llvm::StringSet<> KnownBannedMacroNames = {"EAGAIN", "EWOULDBLOCK", - "SIGCLD", "SIGCHLD"}; + "SIGCLD", "SIGCHLD"}; static bool incrementWithoutOverflow(const APSInt &Value, APSInt &Result) { Result = Value; @@ -101,6 +101,10 @@ 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()) @@ -138,6 +142,23 @@ } } +// Returns the first subexpr of an expr that is not a cast or a copy +// constructor's materialize expr. +// Used by matcher `overloadedOperatorsArgumentsAreEquivalent` for stripping +// unnecessary layers from arguments before deciding whether they are +// equivalent. +const Expr *ignoreCastsAndCopyConstructorCalls(const Expr *CallExpression) { + const Expr *NewExpr; + + if (auto *ConstructorExpr = dyn_cast(CallExpression)) { + if (ConstructorExpr->getNumArgs() == 1) + NewExpr = ConstructorExpr->getArg(0); + } + + NewExpr = NewExpr->IgnoreCasts(); + return NewExpr; +} + // For a given expression 'x', returns whether the ranges covered by the // relational operators are equivalent (i.e. x <= 4 is equivalent to x < 5). static bool areEquivalentRanges(BinaryOperatorKind OpcodeLHS, @@ -313,6 +334,13 @@ areEquivalentExpr(Node.getArg(0), Node.getArg(1)); } +AST_MATCHER(CallExpr, overloadedOperatorsArgumentsAreEquivalent) { + return ( + Node.getNumArgs() == 2 && + areEquivalentExpr(ignoreCastsAndCopyConstructorCalls(Node.getArg(0)), + ignoreCastsAndCopyConstructorCalls(Node.getArg(1)))); +} + AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) { return Node.getOperatorLoc().isMacroID(); } @@ -361,7 +389,7 @@ // Overloaded `retrieveIntegerConstantExpr` for compatibility. static bool retrieveIntegerConstantExpr(const MatchFinder::MatchResult &Result, StringRef Id, APSInt &Value) { - const Expr* ConstExpr = nullptr;; + const Expr *ConstExpr = nullptr; return retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr); } @@ -422,6 +450,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), @@ -449,12 +478,85 @@ 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 isParamNonConstReferenceType(QualType ParamType) { + if (ParamType->isReferenceType() && + !ParamType.getNonReferenceType().isConstQualified()) + return true; + return false; +} + +// 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, + int ParamCount, int ParamsToCheckCount) { + // 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 && + !cast(OperatorDecl->getType().getTypePtr())->isConst()) + return true; + + if (isParamNonConstReferenceType(OperatorDecl->getParamDecl(0)->getType())) + return true; + + if ((ParamsToCheckCount == 2 && ParamCount == 2) && + isParamNonConstReferenceType(OperatorDecl->getParamDecl(1)->getType())) + return true; + + return false; +} + +// OverloadedOperatorKind -> BinaryOperatorKind conversion. +static bool convertOverloadedOpToBinOp(OverloadedOperatorKind OverloadedOp, + BinaryOperatorKind &BinaryOp) { + switch (OverloadedOp) { + case OO_EqualEqual: + BinaryOp = BO_EQ; + break; + case OO_ExclaimEqual: + BinaryOp = BO_NE; + break; + case OO_LessEqual: + BinaryOp = BO_LE; + break; + case OO_GreaterEqual: + BinaryOp = BO_GE; + break; + case OO_Less: + BinaryOp = BO_LT; + break; + case OO_Greater: + BinaryOp = BO_GT; + break; + default: + return false; + } + return true; +} + +// 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, @@ -462,6 +564,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. @@ -470,12 +573,33 @@ 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 = + cast(OverloadedOperatorExpr->getCalleeDecl()); + if (!OverloadedFunctionDecl || OverloadedOperatorExpr->getNumArgs() != 2) + return false; + + if (canOverloadedOperatorArgsBeModified( + OverloadedFunctionDecl, OverloadedFunctionDecl->getNumParams(), 1)) + return false; + + if (!OverloadedOperatorExpr->getArg(1)->isIntegerConstantExpr( + Value, *Result.Context)) + return false; + + Symbol = + ignoreCastsAndCopyConstructorCalls(OverloadedOperatorExpr->getArg(0)); + + OperandExpr = OverloadedOperatorExpr; + + return convertOverloadedOpToBinOp(OverloadedOperatorExpr->getOperator(), + Opcode); } else { return false; } @@ -487,18 +611,18 @@ Opcode = BinaryOperator::reverseComparisonOp(Opcode); if (Result.Nodes.getNodeAs(NegateId)) Opcode = BinaryOperator::negateComparisonOp(Opcode); + return true; } // Checks for expressions like (X == 4) && (Y != 9) -static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const ASTContext *AstCtx) { +static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, + const ASTContext *AstCtx) { if (!isa(BinOp->getLHS()) || !isa(BinOp->getRHS())) return false; - - BinaryOperator* LhsBinOp = dyn_cast(BinOp->getLHS()); - BinaryOperator* RhsBinOp = dyn_cast(BinOp->getRHS()); - + BinaryOperator *LhsBinOp = dyn_cast(BinOp->getLHS()); + BinaryOperator *RhsBinOp = dyn_cast(BinOp->getRHS()); if ((LhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) || LhsBinOp->getRHS()->isIntegerConstantExpr(*AstCtx)) && (RhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) || @@ -565,7 +689,8 @@ return true; } -static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, const Expr *&RhsExpr) { +static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, + const Expr *&RhsExpr) { if (!LhsExpr || !RhsExpr) return false; @@ -593,7 +718,6 @@ static bool isOperatorSetMeaningful(BinaryOperatorKind &Opcode, BinaryOperatorKind &LhsOpcode, BinaryOperatorKind &RhsOpcode) { - if (!BinaryOperator::isLogicalOp(Opcode)) return false; @@ -652,7 +776,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( @@ -674,13 +799,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( @@ -694,7 +818,7 @@ hasOverloadedOperatorName(">"), hasOverloadedOperatorName(">="), hasOverloadedOperatorName("&&"), hasOverloadedOperatorName("||"), hasOverloadedOperatorName("=")), - parametersAreEquivalent(), + overloadedOperatorsArgumentsAreEquivalent(), // Filter noisy false positives. unless(isMacro()), unless(isInTemplateInstantiation())) .bind("call"), @@ -873,6 +997,7 @@ std::swap(LhsOpcode, RhsOpcode); } + // Do not warn if macro expr can be considered intentional. if (isIntendedMacroExpression(Opcode, LhsOpcode, LhsConst, RhsOpcode, RhsConst, Result.Context)) return; @@ -910,17 +1035,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 (isIntendedMacroExpression(MainOpcode, SideOpcode, LhsConst, SideOpcode, RhsConst, Result.Context)) @@ -931,7 +1054,6 @@ } if (const auto *CondOp = Result.Nodes.getNodeAs("cond")) { - const auto *TrueExpr = CondOp->getTrueExpr(); const auto *FalseExpr = CondOp->getFalseExpr(); @@ -942,7 +1064,18 @@ "'true' and 'false' expressions are equivalent"); } + // Check overloaded operators with equivalent operands. if (const auto *Call = Result.Nodes.getNodeAs("call")) { + const auto *OverloadedFunctionDecl = + cast(Call->getCalleeDecl()); + + if (!OverloadedFunctionDecl || Call->getNumArgs() != 2) + return; + + if (canOverloadedOperatorArgsBeModified( + OverloadedFunctionDecl, OverloadedFunctionDecl->getNumParams(), 2)) + 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 comparing 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>=(MyStruct rhs) const { rhs.x++; 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, MyStruct rhs) { rhs.x++; return lhs.x < rhs.x; } // not modifing +bool operator>(const MyStruct& lhs, MyStruct& rhs) { 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(); } @@ -235,7 +255,6 @@ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true if (X + 1 == X - (~0U)) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true - if (X + 1 == X - (~0ULL)) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true @@ -249,7 +268,6 @@ } int TestBitwise(int X, int Y) { - if ((X & 0xFF) == 0xF00) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false if ((X & 0xFF) != 0xF00) return 1; @@ -281,6 +299,34 @@ return 0; } +// Overloaded operators comparing an instance of a struct and an integer constant. +// Overloaded comparison operators without any possible side effect. +struct S { + S(){x = 1;} + int x; +bool operator==(const int &i) const {return x == i;} +bool operator!=(int i) const {return x != i;} +bool operator>(const int& i) const {return x > i;} +bool operator<(int i) const {return x < i;} +}; + +bool operator<=(const S &s, int i) {return s.x <= i;} +bool operator>=(S s, const int &i) {s.x--; return s.x >= i;} + +// Overloaded comparison operators that are able to modify their params, +// therefore may have side effects. +struct S2 { + S2(){x = 1;} + int x; + 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 @@ -331,6 +377,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) {