Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp +++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp @@ -18,7 +18,9 @@ #include "llvm/ADT/APInt.h" #include "llvm/ADT/APSInt.h" #include "llvm/ADT/FoldingSet.h" +#include "llvm/ADT/SmallBitVector.h" #include "llvm/Support/Casting.h" +#include "llvm/Support/FormatVariadic.h" #include #include #include @@ -304,6 +306,132 @@ } } +// to use in the template below +static OverloadedOperatorKind getOp(const BinaryOperator *Op) { + return BinaryOperator::getOverloadedOperator(Op->getOpcode()); +} + +static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) { + if (Op->getNumArgs() != 2) + return OO_None; + return Op->getOperator(); +} + +static std::pair +getOperands(const BinaryOperator *Op) { + return {Op->getLHS()->IgnoreParenImpCasts(), + Op->getRHS()->IgnoreParenImpCasts()}; +} + +static std::pair +getOperands(const CXXOperatorCallExpr *Op) { + return {Op->getArg(0)->IgnoreParenImpCasts(), + Op->getArg(1)->IgnoreParenImpCasts()}; +} + +template +static const TExpr *checkOpKind(const Expr *TheExpr, + OverloadedOperatorKind OpKind) { + const auto *AsTExpr = dyn_cast_or_null(TheExpr); + if (AsTExpr && getOp(AsTExpr) == OpKind) + return AsTExpr; + + return nullptr; +} + +// returns true if a subexpression has two directly equivalent operands and +// is already handled by operands/parametersAreEquivalent +template +static bool collectOperands(const Expr *Part, + SmallVector &AllOperands, + OverloadedOperatorKind OpKind) { + if (const auto *BinOp = checkOpKind(Part, OpKind)) { + const std::pair Operands = getOperands(BinOp); + if (areEquivalentExpr(Operands.first, Operands.second)) + return true; + return collectOperands(Operands.first, AllOperands, OpKind) || + collectOperands(Operands.second, AllOperands, OpKind); + } + + AllOperands.push_back(Part); + return false; +} + +template +static bool hasSameOperatorParent(const Expr *TheExpr, + OverloadedOperatorKind OpKind, + ASTContext &Context) { + // IgnoreParenImpCasts logic in reverse: skip surrounding uninteresting nodes + const DynTypedNodeList Parents = Context.getParents(*TheExpr); + for (ast_type_traits::DynTypedNode DynParent : Parents) { + if (const auto *Parent = DynParent.get()) { + bool Skip = isa(Parent) || isa(Parent) || + isa(Parent) || + isa(Parent); + if (Skip && hasSameOperatorParent(Parent, OpKind, Context)) + return true; + if (checkOpKind(Parent, OpKind)) + return true; + } + } + + return false; +} + +template +static bool +markDuplicateOperands(const TExpr *TheExpr, + ast_matchers::internal::BoundNodesTreeBuilder *Builder, + ASTContext &Context) { + const OverloadedOperatorKind OpKind = getOp(TheExpr); + if (OpKind == OO_None) + return false; + // if there are no nested operators of the same kind, it's handled by + // operands/parametersAreEquivalent + const std::pair Operands = getOperands(TheExpr); + if (!(checkOpKind(Operands.first, OpKind) || + checkOpKind(Operands.second, OpKind))) + return false; + + // if parent is the same kind of operator, it's handled by a previous call to + // markDuplicateOperands + if (hasSameOperatorParent(TheExpr, OpKind, Context)) + return false; + + SmallVector AllOperands; + if (collectOperands(Operands.first, AllOperands, OpKind)) + return false; + if (collectOperands(Operands.second, AllOperands, OpKind)) + return false; + size_t NumOperands = AllOperands.size(); + llvm::SmallBitVector Duplicates(NumOperands); + for (size_t I = 0; I < NumOperands; I++) { + if (Duplicates[I]) + continue; + bool FoundDuplicates = false; + + for (size_t J = I + 1; J < NumOperands; J++) { + if (AllOperands[J]->HasSideEffects(Context)) + break; + + if (areEquivalentExpr(AllOperands[I], AllOperands[J])) { + FoundDuplicates = true; + Duplicates.set(J); + Builder->setBinding( + SmallString<11>(llvm::formatv("duplicate{0}", J)), + ast_type_traits::DynTypedNode::create(*AllOperands[J])); + } + } + + if (FoundDuplicates) + Builder->setBinding( + SmallString<11>(llvm::formatv("duplicate{0}", I)), + ast_type_traits::DynTypedNode::create(*AllOperands[I])); + } + + return Duplicates.any(); +} + AST_MATCHER(Expr, isIntegerConstantExpr) { if (Node.isInstantiationDependent()) return false; @@ -314,6 +442,10 @@ return areEquivalentExpr(Node.getLHS(), Node.getRHS()); } +AST_MATCHER(BinaryOperator, nestedOperandsAreEquivalent) { + return markDuplicateOperands(&Node, Builder, Finder->getASTContext()); +} + AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) { return areEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr()); } @@ -323,6 +455,10 @@ areEquivalentExpr(Node.getArg(0), Node.getArg(1)); } +AST_MATCHER(CXXOperatorCallExpr, nestedParametersAreEquivalent) { + return markDuplicateOperands(&Node, Builder, Finder->getASTContext()); +} + AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) { return Node.getOperatorLoc().isMacroID(); } @@ -484,8 +620,15 @@ // is a temporary expression. Whether the second parameter is checked is // controlled by the parameter `ParamsToCheckCount`. static bool -canOverloadedOperatorArgsBeModified(const FunctionDecl *OperatorDecl, +canOverloadedOperatorArgsBeModified(const CXXOperatorCallExpr *OperatorCall, bool checkSecondParam) { + const auto *OperatorDecl = + dyn_cast_or_null(OperatorCall->getCalleeDecl()); + // if we can't find the declaration, conservatively assume it can modify + // arguments + if (!OperatorDecl) + return true; + unsigned ParamCount = OperatorDecl->getNumParams(); // Overloaded operators declared inside a class have only one param. @@ -527,14 +670,7 @@ 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 (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, false)) + if (canOverloadedOperatorArgsBeModified(OverloadedOperatorExpr, false)) return false; if (const auto *Arg = OverloadedOperatorExpr->getArg(1)) { @@ -714,6 +850,21 @@ .bind("binary"), this); + // Logical or bitwise operator with equivalent nested operands, like (X && Y + // && X) or (X && (Y && X)) + Finder->addMatcher( + binaryOperator(anyOf(hasOperatorName("|"), hasOperatorName("&"), + hasOperatorName("||"), hasOperatorName("&&"), + hasOperatorName("^")), + nestedOperandsAreEquivalent(), + // Filter noisy false positives. + unless(isInTemplateInstantiation()), + unless(binaryOperatorIsInMacro()), + // TODO: if the banned macros are themselves duplicated + unless(hasDescendant(BannedIntegerLiteral))) + .bind("nested-duplicates"), + this); + // Conditional (trenary) operator with equivalent operands, like (Y ? X : X). Finder->addMatcher(conditionalOperator(expressionsAreEquivalent(), // Filter noisy false positives. @@ -740,6 +891,19 @@ .bind("call"), this); + // Overloaded operators with equivalent operands. + Finder->addMatcher( + cxxOperatorCallExpr( + anyOf(hasOverloadedOperatorName("|"), hasOverloadedOperatorName("&"), + hasOverloadedOperatorName("||"), + hasOverloadedOperatorName("&&"), + hasOverloadedOperatorName("^")), + nestedParametersAreEquivalent(), argumentCountIs(2), + // Filter noisy false positives. + unless(isMacro()), unless(isInTemplateInstantiation())) + .bind("nested-duplicates"), + this); + // Match expressions like: !(1 | 2 | 3) Finder->addMatcher( implicitCastExpr( @@ -1061,17 +1225,29 @@ } if (const auto *Call = Result.Nodes.getNodeAs("call")) { - const auto *OverloadedFunctionDecl = dyn_cast_or_null(Call->getCalleeDecl()); - if (!OverloadedFunctionDecl) - return; - - if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, true)) + if (canOverloadedOperatorArgsBeModified(Call, true)) return; diag(Call->getOperatorLoc(), "both sides of overloaded operator are equivalent"); } + if (const auto *Op = Result.Nodes.getNodeAs("nested-duplicates")) { + const auto *Call = dyn_cast(Op); + if (Call && canOverloadedOperatorArgsBeModified(Call, true)) + return; + + StringRef Message = + Call ? "overloaded operator has equivalent nested operands" + : "operator has equivalent nested operands"; + + const auto Diag = diag(Op->getExprLoc(), Message); + for (const auto &KeyValue : Result.Nodes.getMap()) { + if (StringRef(KeyValue.first).startswith("duplicate")) + Diag << KeyValue.second.getSourceRange(); + } + } + if (const auto *NegateOperator = Result.Nodes.getNodeAs("logical-bitwise-confusion")) { SourceLocation OperatorLoc = NegateOperator->getOperatorLoc(); Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp +++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp @@ -81,6 +81,13 @@ if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent + if (X && Y && X) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: operator has equivalent nested operands + if (X || (Y || X)) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: operator has equivalent nested operands + if ((X ^ Y) ^ (Y ^ X)) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: operator has equivalent nested operands + return 0; } @@ -106,6 +113,7 @@ if (++X != ++X) return 1; if (P.a[X]++ != P.a[X]++) return 1; if (P.a[X++] != P.a[X++]) return 1; + if (X && X++ && X) return 1; if ("abc" == "ABC") return 1; if (foo(bar(0)) < (foo(bat(0, 1)))) return 1; @@ -163,6 +171,15 @@ 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; } +struct MyStruct1 { + bool x; + MyStruct1(bool x) : x(x) {}; + operator bool() { return x; } +}; + +MyStruct1 operator&&(const MyStruct1& lhs, const MyStruct1& rhs) { return lhs.x && rhs.x; } +MyStruct1 operator||(MyStruct1& lhs, MyStruct1& rhs) { return lhs.x && rhs.x; } + bool TestOverloadedOperator(MyStruct& S) { if (S == Q) return false; @@ -180,6 +197,15 @@ if (S >= S) return true; // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent + MyStruct1 U(false); + MyStruct1 V(true); + + // valid because the operator is not const + if ((U || V) || U) return true; + + if (U && V && U && V) return true; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: overloaded operator has equivalent nested operands + return true; }