diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp --- a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp +++ b/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,114 @@ } } +// 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) { + const auto *PartAsBinOp = checkOpKind(Part, OpKind); + if (PartAsBinOp) { + auto Operands = getOperands(PartAsBinOp); + if (areEquivalentExpr(Operands.first, Operands.second)) + return true; + return collectOperands(Operands.first, AllOperands, OpKind) || + collectOperands(Operands.second, AllOperands, OpKind); + } else { + AllOperands.push_back(Part); + } + 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 + const ASTContext::DynTypedNodeList Parents = Context.getParents(*TheExpr); + for (ast_type_traits::DynTypedNode Parent : Parents) { + if (checkOpKind(Parent.get(), OpKind)) + 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 +424,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 +437,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 +602,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 +652,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 +832,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 +873,20 @@ .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 +1208,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(); diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp @@ -81,6 +81,11 @@ 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 + return 0; } @@ -106,6 +111,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;