Author: xazax Date: Mon Nov 27 07:05:24 2017 New Revision: 319033 URL: http://llvm.org/viewvc/llvm-project?rev=319033&view=rev Log: [clang-tidy] Misc redundant expressions check updated for overloaded operators
Patch by: Lilla Barancsuk Differential Revision: https://reviews.llvm.org/D39243 Modified: clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp Modified: clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp?rev=319033&r1=319032&r2=319033&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp Mon Nov 27 07:05:24 2017 @@ -98,6 +98,9 @@ static bool areEquivalentExpr(const Expr case Stmt::StringLiteralClass: return cast<StringLiteral>(Left)->getBytes() == cast<StringLiteral>(Right)->getBytes(); + case Stmt::CXXOperatorCallExprClass: + return cast<CXXOperatorCallExpr>(Left)->getOperator() == + cast<CXXOperatorCallExpr>(Right)->getOperator(); case Stmt::DependentScopeDeclRefExprClass: if (cast<DependentScopeDeclRefExpr>(Left)->getDeclName() != cast<DependentScopeDeclRefExpr>(Right)->getDeclName()) @@ -410,6 +413,7 @@ matchRelationalIntegerConstantExpr(Strin 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 @@ matchRelationalIntegerConstantExpr(Strin 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<FunctionType>()->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 @@ static bool retrieveRelationalIntegerCon 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<BinaryOperator>(Id)) { // Operand received with explicit comparator. @@ -458,12 +505,29 @@ static bool retrieveRelationalIntegerCon if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr)) return false; - } else if (const auto *Cast = Result.Nodes.getNodeAs<CastExpr>(CastId)) { // Operand received with implicit comparator (cast). Opcode = BO_NE; OperandExpr = Cast; Value = APSInt(32, false); + } else if (const auto *OverloadedOperatorExpr = + Result.Nodes.getNodeAs<CXXOperatorCallExpr>(OverloadId)) { + const auto *OverloadedFunctionDecl = dyn_cast_or_null<FunctionDecl>(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 @@ static bool areExprsFromDifferentMacros( 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 @@ void RedundantExpressionCheck::registerM 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 @@ void RedundantExpressionCheck::registerM 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::checkRela void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("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 @@ void RedundantExpressionCheck::check(con } if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call")) { + const auto *OverloadedFunctionDecl = dyn_cast_or_null<FunctionDecl>(Call->getCalleeDecl()); + if (!OverloadedFunctionDecl) + return; + + if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, true)) + return; + diag(Call->getOperatorLoc(), "both sides of overloaded operator are equivalent"); } Modified: clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp?rev=319033&r1=319032&r2=319033&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp Mon Nov 27 07:05:24 2017 @@ -124,16 +124,36 @@ int TestConditional(int x, int y) { #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 @@ template <typename T, typename U> 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<MyClass, MyClass>(); } @@ -281,9 +301,46 @@ int TestBitwise(int X, int Y) { 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 @@ int TestLogical(int X, int Y){ 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) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
