Index: clang-tidy/misc/RedundantExpressionCheck.cpp =================================================================== --- clang-tidy/misc/RedundantExpressionCheck.cpp +++ clang-tidy/misc/RedundantExpressionCheck.cpp @@ -11,6 +11,7 @@ #include "../utils/Matchers.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" using namespace clang::ast_matchers; @@ -18,7 +19,31 @@ namespace tidy { namespace misc { -static bool AreIdenticalExpr(const Expr *Left, const Expr *Right) { +static const char KnownBannedMacroNames[] = + "EAGAIN;EWOULDBLOCK;SIGCLD;SIGCHLD;"; + +static const char MacroNamesDelimiter[] = ";"; + +static void ParseMacroNames(StringRef Option, + std::vector *Result) { + SmallVector Names; + Option.split(Names, MacroNamesDelimiter); + for (StringRef &Name : Names) { + Name = Name.trim(); + if (!Name.empty()) + Result->push_back(Name); + } +} + +static bool AreEquivalentNameSpecifier(const NestedNameSpecifier *Left, + const NestedNameSpecifier *Right) { + llvm::FoldingSetNodeID LeftID, RightID; + Left->Profile(LeftID); + Right->Profile(RightID); + return LeftID == RightID; +} + +static bool AreEquivalentExpr(const Expr *Left, const Expr *Right) { if (!Left || !Right) return !Left && !Right; @@ -33,8 +58,8 @@ Expr::const_child_iterator LeftIter = Left->child_begin(); Expr::const_child_iterator RightIter = Right->child_begin(); while (LeftIter != Left->child_end() && RightIter != Right->child_end()) { - if (!AreIdenticalExpr(dyn_cast(*LeftIter), - dyn_cast(*RightIter))) + if (!AreEquivalentExpr(dyn_cast(*LeftIter), + dyn_cast(*RightIter))) return false; ++LeftIter; ++RightIter; @@ -53,7 +78,8 @@ case Stmt::IntegerLiteralClass: { llvm::APInt LeftLit = cast(Left)->getValue(); llvm::APInt RightLit = cast(Right)->getValue(); - return LeftLit.getBitWidth() == RightLit.getBitWidth() && LeftLit == RightLit; + return LeftLit.getBitWidth() == RightLit.getBitWidth() && + LeftLit == RightLit; } case Stmt::FloatingLiteralClass: return cast(Left)->getValue().bitwiseIsEqual( @@ -62,6 +88,13 @@ return cast(Left)->getBytes() == cast(Right)->getBytes(); + case Stmt::DependentScopeDeclRefExprClass: + if (cast(Left)->getDeclName() != + cast(Right)->getDeclName()) + return false; + return AreEquivalentNameSpecifier( + cast(Left)->getQualifier(), + cast(Right)->getQualifier()); case Stmt::DeclRefExprClass: return cast(Left)->getDecl() == cast(Right)->getDecl(); @@ -89,22 +122,51 @@ } } -AST_MATCHER(BinaryOperator, OperandsAreEquivalent) { - return AreIdenticalExpr(Node.getLHS(), Node.getRHS()); +AST_MATCHER(BinaryOperator, operandsAreEquivalent) { + return AreEquivalentExpr(Node.getLHS(), Node.getRHS()); +} + +AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) { + return AreEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr()); +} + +AST_MATCHER(CallExpr, parametersAreEquivalent) { + return Node.getNumArgs() == 2 && + AreEquivalentExpr(Node.getArg(0), Node.getArg(1)); } -AST_MATCHER(BinaryOperator, isInMacro) { +AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) { return Node.getOperatorLoc().isMacroID(); } -AST_MATCHER(Expr, isInstantiationDependent) { - return Node.isInstantiationDependent(); +AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) { + return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID(); +} + +AST_MATCHER(Expr, isMacro) { return Node.getExprLoc().isMacroID(); } + +AST_MATCHER_P(Expr, expandedByMacro, std::vector, NameRefs) { + const SourceManager &SM = Finder->getASTContext().getSourceManager(); + const LangOptions &LO = Finder->getASTContext().getLangOpts(); + std::set Names(NameRefs.begin(), NameRefs.end()); + SourceLocation Loc = Node.getExprLoc(); + while (Loc.isMacroID()) { + std::string MacroName = Lexer::getImmediateMacroName(Loc, SM, LO); + if (Names.find(MacroName) != Names.end()) + return true; + Loc = SM.getImmediateMacroCallerLoc(Loc); + } + return false; } void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) { const auto AnyLiteralExpr = ignoringParenImpCasts( anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral())); + std::vector MacroNames; + ParseMacroNames(KnownBannedMacroNames, &MacroNames); + const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(MacroNames)); + Finder->addMatcher( binaryOperator(anyOf(hasOperatorName("-"), hasOperatorName("/"), hasOperatorName("%"), hasOperatorName("|"), @@ -112,20 +174,52 @@ matchers::isComparisonOperator(), hasOperatorName("&&"), hasOperatorName("||"), hasOperatorName("=")), - OperandsAreEquivalent(), + operandsAreEquivalent(), // Filter noisy false positives. - unless(isInstantiationDependent()), - unless(isInMacro()), + unless(isInTemplateInstantiation()), + unless(binaryOperatorIsInMacro()), unless(hasType(realFloatingPointType())), unless(hasEitherOperand(hasType(realFloatingPointType()))), - unless(hasLHS(AnyLiteralExpr))) + unless(hasLHS(AnyLiteralExpr)), + unless(hasDescendant(BannedIntegerLiteral))) .bind("binary"), this); + + Finder->addMatcher( + conditionalOperator(expressionsAreEquivalent(), + // Filter noisy false positives. + unless(conditionalOperatorIsInMacro()), + unless(hasTrueExpression(AnyLiteralExpr)), + unless(isInTemplateInstantiation())) + .bind("cond"), + this); + + Finder->addMatcher( + cxxOperatorCallExpr( + anyOf( + hasOverloadedOperatorName("-"), hasOverloadedOperatorName("/"), + hasOverloadedOperatorName("%"), hasOverloadedOperatorName("|"), + hasOverloadedOperatorName("&"), hasOverloadedOperatorName("^"), + hasOverloadedOperatorName("=="), hasOverloadedOperatorName("!="), + hasOverloadedOperatorName("<"), hasOverloadedOperatorName("<="), + hasOverloadedOperatorName(">"), hasOverloadedOperatorName(">="), + hasOverloadedOperatorName("&&"), hasOverloadedOperatorName("||"), + hasOverloadedOperatorName("=")), + parametersAreEquivalent(), + // Filter noisy false positives. + unless(isMacro()), unless(isInTemplateInstantiation())) + .bind("call"), + this); } void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *BinOp = Result.Nodes.getNodeAs("binary")) diag(BinOp->getOperatorLoc(), "both side of operator are equivalent"); + if (const auto *CondOp = Result.Nodes.getNodeAs("cond")) + diag(CondOp->getColonLoc(), "'true' and 'false' expression are equivalent"); + if (const auto *Call = Result.Nodes.getNodeAs("call")) + diag(Call->getOperatorLoc(), + "both side of overloaded operator are equivalent"); } } // namespace misc Index: docs/clang-tidy/checks/misc-redundant-expression.rst =================================================================== --- docs/clang-tidy/checks/misc-redundant-expression.rst +++ docs/clang-tidy/checks/misc-redundant-expression.rst @@ -12,6 +12,7 @@ * always be a constant (zero or one) Example: + .. code:: c++ ((x+1) | (x+1)) // (x+1) is redundant Index: test/clang-tidy/misc-redundant-expression.cpp =================================================================== --- test/clang-tidy/misc-redundant-expression.cpp +++ test/clang-tidy/misc-redundant-expression.cpp @@ -68,14 +68,14 @@ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent if (foo(0) - 2 < foo(0) - 2) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent if (foo(bar(0)) < (foo(bar((0))))) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent if (P1.x < P2.x && P1.x < P2.x) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both side of operator are equivalent return 0; } @@ -100,13 +100,36 @@ return 0; } +int TestConditional(int x, int y) { + int k = 0; + k += (y < 0) ? x : x; + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 'true' and 'false' expression are equivalent + k += (y < 0) ? x + 1 : x + 1; + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'true' and 'false' expression are equivalent + return k; +} + +struct MyStruct { + int x; +} Q; +bool operator==(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; } +bool TestOperator(const MyStruct& S) { + if (S == Q) return false; + return S == S; + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: both side of overloaded operator are equivalent +} + #define LT(x, y) (void)((x) < (y)) +#define COND(x, y, z) ((x)?(y):(z)) +#define EQUALS(x, y) (x) == (y) int TestMacro(int X, int Y) { LT(0, 0); LT(1, 0); LT(X, X); LT(X+1, X + 1); + COND(X < Y, X, X); + EQUALS(Q, Q); } int TestFalsePositive(int* A, int X, float F) { @@ -118,3 +141,22 @@ if (F != F && F == F) return 1; return 0; } + +int TestBannedMacros() { +#define EAGAIN 3 +#define NOT_EAGAIN 3 + if (EAGAIN == 0 | EAGAIN == 0) return 0; + if (NOT_EAGAIN == 0 | NOT_EAGAIN == 0) return 0; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: both side of operator are equivalent +} + +struct MyClass { +static const int Value = 42; +}; +template +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 side of overloaded operator are equivalent +} +void TestTemplate() { TemplateCheck(); }