This revision was automatically updated to reflect the committed changes. Closed by commit rL317570: [clang-tidy] Misc redundant expressions checker updated for macros (authored by xazax).
Changed prior to commit: https://reviews.llvm.org/D38688?vs=121740&id=121875#toc Repository: rL LLVM https://reviews.llvm.org/D38688 Files: clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp
Index: clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp +++ clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp @@ -15,69 +15,71 @@ extern int bar(int x); extern int bat(int x, int y); -int Test(int X, int Y) { +int TestSimpleEquivalent(int X, int Y) { if (X - X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent [misc-redundant-expression] + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent [misc-redundant-expression] if (X / X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X % X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X & X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X | X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X ^ X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X < X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X <= X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X > X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X >= X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X && X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X || X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X != (((X)))) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent if (X + 1 == X + 1) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent if (X + 1 != X + 1) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent if (X + 1 <= X + 1) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent if (X + 1 >= X + 1) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent if ((X != 1 || Y != 1) && (X != 1 || Y != 1)) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent if (P.a[X - P.x] != P.a[X - P.x]) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both sides of operator are equivalent if ((int)X < (int)X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent + if (int(X) < int(X)) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent if ( + "dummy" == + "dummy") return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both sides of operator are equivalent if (L"abc" == L"abc") return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides 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 sides 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 sides 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 sides 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 sides of operator are equivalent return 0; } @@ -102,23 +104,36 @@ return 0; } +#define COND_OP_MACRO 9 +#define COND_OP_OTHER_MACRO 9 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 + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 'true' and 'false' expressions are equivalent k += (y < 0) ? x + 1 : x + 1; - // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'true' and 'false' expression are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'true' and 'false' expressions are equivalent + k += (y < 0) ? COND_OP_MACRO : COND_OP_MACRO; + // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: 'true' and 'false' expressions are equivalent + + // Do not match for conditional operators with a macro and a const. + k += (y < 0) ? COND_OP_MACRO : 9; + // Do not match for conditional operators with expressions from different macros. + k += (y < 0) ? COND_OP_MACRO : COND_OP_OTHER_MACRO; return k; } +#undef COND_OP_MACRO +#undef COND_OP_OTHER_MACRO struct MyStruct { int x; } Q; + bool operator==(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; } -bool TestOperator(const MyStruct& S) { + +bool TestOperator(MyStruct& S) { if (S == Q) return false; - return S == S; - // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: both side 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)) @@ -132,6 +147,7 @@ LT(X+1, X + 1); COND(X < Y, X, X); EQUALS(Q, Q); + return 0; } int TestFalsePositive(int* A, int X, float F) { @@ -149,7 +165,8 @@ #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 + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: both sides of operator are equivalent + return 0; } struct MyClass { @@ -159,7 +176,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 side of overloaded operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of overloaded operator are equivalent } void TestTemplate() { TemplateCheck<MyClass, MyClass>(); } @@ -218,6 +235,7 @@ // 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 @@ -230,7 +248,8 @@ return 0; } -int TestBitwise(int X) { +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; @@ -262,7 +281,14 @@ return 0; } -int TestRelational(int X, int Y) { +int TestLogical(int X, int Y){ +#define CONFIG 0 + if (CONFIG && X) return 1; // OK, consts from macros are considered intentional +#undef CONFIG +#define CONFIG 1 + if (CONFIG || X) return 1; +#undef CONFIG + if (X == 10 && X != 10) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false if (X == 10 && (X != 10)) return 1; @@ -289,14 +315,25 @@ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false if (X && !!X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: equivalent expression on both side of logical operator + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: equivalent expression on both sides of logical operator if (X != 0 && X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator if (X != 0 && !!X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator if (X == 0 && !X) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator + + // Should not match. + if (X == 10 && Y == 10) return 1; + if (X != 10 && X != 12) return 1; + if (X == 10 || X == 12) return 1; + if (!X && !Y) return 1; + if (!X && Y) return 1; + if (!X && Y == 0) return 1; + if (X == 10 && Y != 10) return 1; +} +int TestRelational(int X, int Y) { if (X == 10 && X > 10) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false if (X == 10 && X < 10) return 1; @@ -323,18 +360,22 @@ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true if (X != 7 || X != 14) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always true + if (X == 7 || X != 5) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X != 7 || X == 7) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always true if (X < 7 && X < 6) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant if (X < 7 && X < 7) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent if (X < 7 && X < 8) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant if (X < 7 && X <= 5) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant if (X < 7 && X <= 6) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: equivalent expression on both side of logical operator + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: equivalent expression on both sides of logical operator if (X < 7 && X <= 7) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant if (X < 7 && X <= 8) return 1; @@ -345,14 +386,21 @@ if (X <= 7 && X < 7) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant if (X <= 7 && X < 8) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator + + if (X >= 7 && X > 6) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator + if (X >= 7 && X > 7) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X >= 7 && X > 8) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant if (X <= 7 && X <= 5) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant if (X <= 7 && X <= 6) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant if (X <= 7 && X <= 7) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent if (X <= 7 && X <= 8) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: expression is redundant @@ -379,14 +427,18 @@ if (X < 7 || X < 6) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant if (X < 7 || X < 7) return 1; - // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent if (X < 7 || X < 8) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X > 7 || X > 6) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X > 7 || X > 7) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent + if (X > 7 || X > 8) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant + // Should not match. - if (X == 10 && Y == 10) return 1; - if (X != 10 && X != 12) return 1; - if (X == 10 || X == 12) return 1; if (X < 10 || X > 12) return 1; if (X > 10 && X < 12) return 1; if (X < 10 || X >= 12) return 1; @@ -401,12 +453,56 @@ if (X > 10 && X != 11) return 1; if (X >= 10 && X <= 10) return 1; if (X <= 10 && X >= 10) return 1; - if (!X && !Y) return 1; - if (!X && Y) return 1; - if (!X && Y == 0) return 1; - if (X == 10 && Y != 10) return 1; if (X < 0 || X > 0) return 1; +} +int TestRelationalMacros(int X){ +#define SOME_MACRO 3 +#define SOME_MACRO_SAME_VALUE 3 +#define SOME_OTHER_MACRO 9 + // Do not match for redundant relational macro expressions that can be + // considered intentional, and for some particular values, non redundant. + + // Test cases for expressions with the same macro on both sides. + if (X < SOME_MACRO && X > SOME_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: logical expression is always false + if (X < SOME_MACRO && X == SOME_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: logical expression is always false + if (X < SOME_MACRO || X >= SOME_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: logical expression is always true + if (X <= SOME_MACRO || X > SOME_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always true + if (X != SOME_MACRO && X > SOME_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X != SOME_MACRO && X < SOME_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + + // Test cases for two different macros. + if (X < SOME_MACRO && X > SOME_OTHER_MACRO) return 1; + if (X != SOME_MACRO && X >= SOME_OTHER_MACRO) return 1; + if (X != SOME_MACRO && X != SOME_OTHER_MACRO) return 1; + if (X == SOME_MACRO || X == SOME_MACRO_SAME_VALUE) return 1; + if (X == SOME_MACRO || X <= SOME_MACRO_SAME_VALUE) return 1; + if (X == SOME_MACRO || X > SOME_MACRO_SAME_VALUE) return 1; + if (X < SOME_MACRO && X <= SOME_OTHER_MACRO) return 1; + if (X == SOME_MACRO && X > SOME_OTHER_MACRO) return 1; + if (X == SOME_MACRO && X != SOME_OTHER_MACRO) return 1; + if (X == SOME_MACRO && X != SOME_MACRO_SAME_VALUE) return 1; + if (X == SOME_MACRO_SAME_VALUE && X == SOME_MACRO ) return 1; + + // Test cases for a macro and a const. + if (X < SOME_MACRO && X > 9) return 1; + if (X != SOME_MACRO && X >= 9) return 1; + if (X != SOME_MACRO && X != 9) return 1; + if (X == SOME_MACRO || X == 3) return 1; + if (X == SOME_MACRO || X <= 3) return 1; + if (X < SOME_MACRO && X <= 9) return 1; + if (X == SOME_MACRO && X != 9) return 1; + if (X == SOME_MACRO && X == 9) return 1; + +#undef SOME_OTHER_MACRO +#undef SOME_MACRO_SAME_VALUE +#undef SOME_MACRO return 0; } @@ -419,7 +515,7 @@ } enum Color { Red, Yellow, Green }; -int TestRelatiopnalWithEnum(enum Color C) { +int TestRelationalWithEnum(enum Color C) { if (C == Red && C == Yellow) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false if (C == Red && C != Red) return 1; @@ -437,7 +533,7 @@ template<class T> int TestRelationalTemplated(int X) { // This test causes a corner case with |isIntegerConstantExpr| where the type - // is dependant. There is an assert failing when evaluating + // is dependent. There is an assert failing when evaluating // sizeof(<incomplet-type>). if (sizeof(T) == 4 || sizeof(T) == 8) return 1; @@ -450,10 +546,13 @@ int TestWithSignedUnsigned(int X) { if (X + 1 == X + 1ULL) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true + if ((X & 0xFFU) == 0xF00) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: logical expression is always false + if ((X & 0xFF) == 0xF00U) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false + if ((X & 0xFFU) == 0xF00U) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: logical expression is always false @@ -476,6 +575,7 @@ if (X <= X + 0xFFFFFFFFU) return 1; if (X <= X + 0x7FFFFFFF) return 1; if (X <= X + 0x80000000) return 1; + if (X <= 0xFFFFFFFFU && X > 0) return 1; if (X <= 0xFFFFFFFFU && X > 0U) return 1; Index: clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp @@ -23,7 +23,6 @@ #include <algorithm> #include <cassert> #include <cstdint> -#include <set> #include <string> #include <vector> @@ -38,8 +37,8 @@ using llvm::APSInt; } // namespace -static const char KnownBannedMacroNames[] = - "EAGAIN;EWOULDBLOCK;SIGCLD;SIGCHLD;"; +static const llvm::StringSet<> KnownBannedMacroNames = {"EAGAIN", "EWOULDBLOCK", + "SIGCLD", "SIGCHLD"}; static bool incrementWithoutOverflow(const APSInt &Value, APSInt &Result) { Result = Value; @@ -99,7 +98,6 @@ case Stmt::StringLiteralClass: return cast<StringLiteral>(Left)->getBytes() == cast<StringLiteral>(Right)->getBytes(); - case Stmt::DependentScopeDeclRefExprClass: if (cast<DependentScopeDeclRefExpr>(Left)->getDeclName() != cast<DependentScopeDeclRefExpr>(Right)->getDeclName()) @@ -113,16 +111,14 @@ case Stmt::MemberExprClass: return cast<MemberExpr>(Left)->getMemberDecl() == cast<MemberExpr>(Right)->getMemberDecl(); - + case Stmt::CXXFunctionalCastExprClass: case Stmt::CStyleCastExprClass: - return cast<CStyleCastExpr>(Left)->getTypeAsWritten() == - cast<CStyleCastExpr>(Right)->getTypeAsWritten(); - + return cast<ExplicitCastExpr>(Left)->getTypeAsWritten() == + cast<ExplicitCastExpr>(Right)->getTypeAsWritten(); case Stmt::CallExprClass: case Stmt::ImplicitCastExprClass: case Stmt::ArraySubscriptExprClass: return true; - case Stmt::UnaryOperatorClass: if (cast<UnaryOperator>(Left)->isIncrementDecrementOp()) return false; @@ -282,7 +278,8 @@ } } -static void canonicalNegateExpr(BinaryOperatorKind &Opcode, APSInt &Value) { +static void transformSubToCanonicalAddExpr(BinaryOperatorKind &Opcode, + APSInt &Value) { if (Opcode == BO_Sub) { Opcode = BO_Add; Value = -Value; @@ -295,32 +292,77 @@ return Node.isIntegerConstantExpr(Finder->getASTContext()); } -// Returns a matcher for integer constant expression. +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, binaryOperatorIsInMacro) { + return Node.getOperatorLoc().isMacroID(); +} + +AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) { + return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID(); +} + +AST_MATCHER(Expr, isMacro) { return Node.getExprLoc().isMacroID(); } + +AST_MATCHER_P(Expr, expandedByMacro, llvm::StringSet<>, Names) { + const SourceManager &SM = Finder->getASTContext().getSourceManager(); + const LangOptions &LO = Finder->getASTContext().getLangOpts(); + SourceLocation Loc = Node.getExprLoc(); + while (Loc.isMacroID()) { + StringRef MacroName = Lexer::getImmediateMacroName(Loc, SM, LO); + if (Names.count(MacroName)) + return true; + Loc = SM.getImmediateMacroCallerLoc(Loc); + } + return false; +} + +// Returns a matcher for integer constant expressions. static ast_matchers::internal::Matcher<Expr> matchIntegerConstantExpr(StringRef Id) { std::string CstId = (Id + "-const").str(); return expr(isIntegerConstantExpr()).bind(CstId); } -// Retrieve the integer value matched by 'matchIntegerConstantExpr' with name -// 'Id' and store it into 'Value'. +// Retrieves the integer expression matched by 'matchIntegerConstantExpr' with +// name 'Id' and stores it into 'ConstExpr', the value of the expression is +// stored into `Value`. static bool retrieveIntegerConstantExpr(const MatchFinder::MatchResult &Result, - StringRef Id, APSInt &Value) { + StringRef Id, APSInt &Value, + const Expr *&ConstExpr) { std::string CstId = (Id + "-const").str(); - const auto *CstExpr = Result.Nodes.getNodeAs<Expr>(CstId); - return CstExpr && CstExpr->isIntegerConstantExpr(Value, *Result.Context); + ConstExpr = Result.Nodes.getNodeAs<Expr>(CstId); + return ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context); +} + +// Overloaded `retrieveIntegerConstantExpr` for compatibility. +static bool retrieveIntegerConstantExpr(const MatchFinder::MatchResult &Result, + StringRef Id, APSInt &Value) { + const Expr *ConstExpr = nullptr; + return retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr); } -// Returns a matcher for a symbolic expression (any expression except ingeter -// constant expression). +// Returns a matcher for symbolic expressions (matches every expression except +// ingeter constant expressions). static ast_matchers::internal::Matcher<Expr> matchSymbolicExpr(StringRef Id) { std::string SymId = (Id + "-sym").str(); return ignoringParenImpCasts( expr(unless(isIntegerConstantExpr())).bind(SymId)); } -// Retrieve the expression matched by 'matchSymbolicExpr' with name 'Id' and -// store it into 'SymExpr'. +// Retrieves the expression matched by 'matchSymbolicExpr' with name 'Id' and +// stores it into 'SymExpr'. static bool retrieveSymbolicExpr(const MatchFinder::MatchResult &Result, StringRef Id, const Expr *&SymExpr) { std::string SymId = (Id + "-sym").str(); @@ -348,7 +390,7 @@ return ignoringParenImpCasts(BinOpCstExpr); } -// Retrieve sub-expressions matched by 'matchBinOpIntegerConstantExpr' with +// Retrieves sub-expressions matched by 'matchBinOpIntegerConstantExpr' with // name 'Id'. static bool retrieveBinOpIntegerConstantExpr(const MatchFinder::MatchResult &Result, @@ -362,7 +404,7 @@ return false; } -// Matches relational expression: 'Expr <op> k' (i.e. x < 2, x != 3, 12 <= x). +// Matches relational expressions: 'Expr <op> k' (i.e. x < 2, x != 3, 12 <= x). static ast_matchers::internal::Matcher<Expr> matchRelationalIntegerConstantExpr(StringRef Id) { std::string CastId = (Id + "-cast").str(); @@ -388,6 +430,7 @@ hasUnaryOperand(anyOf(CastExpr, RelationalExpr))) .bind(NegateId); + // Do not bind to double negation. const auto NegateNegateRelationalExpr = unaryOperator(hasOperatorName("!"), hasUnaryOperand(unaryOperator( @@ -398,23 +441,24 @@ NegateNegateRelationalExpr); } -// Retrieve sub-expressions matched by 'matchRelationalIntegerConstantExpr' with +// 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, APSInt &Value) { +static bool retrieveRelationalIntegerConstantExpr( + const MatchFinder::MatchResult &Result, StringRef Id, + const Expr *&OperandExpr, BinaryOperatorKind &Opcode, const Expr *&Symbol, + APSInt &Value, const Expr *&ConstExpr) { std::string CastId = (Id + "-cast").str(); std::string SwapId = (Id + "-swap").str(); std::string NegateId = (Id + "-negate").str(); if (const auto *Bin = Result.Nodes.getNodeAs<BinaryOperator>(Id)) { // Operand received with explicit comparator. Opcode = Bin->getOpcode(); OperandExpr = Bin; - if (!retrieveIntegerConstantExpr(Result, Id, Value)) + + 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; @@ -431,56 +475,96 @@ Opcode = BinaryOperator::reverseComparisonOp(Opcode); if (Result.Nodes.getNodeAs<Expr>(NegateId)) Opcode = BinaryOperator::negateComparisonOp(Opcode); - return true; } -AST_MATCHER(BinaryOperator, operandsAreEquivalent) { - return areEquivalentExpr(Node.getLHS(), Node.getRHS()); -} +// Checks for expressions like (X == 4) && (Y != 9) +static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const ASTContext *AstCtx) { + const auto *LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS()); + const auto *RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS()); -AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) { - return areEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr()); -} + if (!LhsBinOp || !RhsBinOp) + return false; -AST_MATCHER(CallExpr, parametersAreEquivalent) { - return Node.getNumArgs() == 2 && - areEquivalentExpr(Node.getArg(0), Node.getArg(1)); + if ((LhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) || + LhsBinOp->getRHS()->isIntegerConstantExpr(*AstCtx)) && + (RhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) || + RhsBinOp->getRHS()->isIntegerConstantExpr(*AstCtx))) + return true; + return false; } -AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) { - return Node.getOperatorLoc().isMacroID(); +// Retrieves integer constant subexpressions from binary operator expressions +// that have two equivalent sides +// E.g.: from (X == 5) && (X == 5) retrieves 5 and 5. +static bool retrieveConstExprFromBothSides(const BinaryOperator *&BinOp, + BinaryOperatorKind &MainOpcode, + BinaryOperatorKind &SideOpcode, + const Expr *&LhsConst, + const Expr *&RhsConst, + const ASTContext *AstCtx) { + assert(areSidesBinaryConstExpressions(BinOp, AstCtx) && + "Both sides of binary operator must be constant expressions!"); + + MainOpcode = BinOp->getOpcode(); + + const auto *BinOpLhs = cast<BinaryOperator>(BinOp->getLHS()); + const auto *BinOpRhs = cast<BinaryOperator>(BinOp->getRHS()); + + LhsConst = BinOpLhs->getLHS()->isIntegerConstantExpr(*AstCtx) + ? BinOpLhs->getLHS() + : BinOpLhs->getRHS(); + RhsConst = BinOpRhs->getLHS()->isIntegerConstantExpr(*AstCtx) + ? BinOpRhs->getLHS() + : BinOpRhs->getRHS(); + + if (!LhsConst || !RhsConst) + return false; + + assert(BinOpLhs->getOpcode() == BinOpRhs->getOpcode() && + "Sides of the binary operator must be equivalent expressions!"); + + SideOpcode = BinOpLhs->getOpcode(); + + return true; } -AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) { - return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID(); +static bool areExprsFromDifferentMacros(const Expr *LhsExpr, + const Expr *RhsExpr, + const ASTContext *AstCtx) { + if (!LhsExpr || !RhsExpr) + return false; + + SourceLocation LhsLoc = LhsExpr->getExprLoc(); + SourceLocation RhsLoc = RhsExpr->getExprLoc(); + + if (!LhsLoc.isMacroID() || !RhsLoc.isMacroID()) + return false; + + const SourceManager &SM = AstCtx->getSourceManager(); + const LangOptions &LO = AstCtx->getLangOpts(); + + return !(Lexer::getImmediateMacroName(LhsLoc, SM, LO) == + Lexer::getImmediateMacroName(RhsLoc, SM, LO)); } -AST_MATCHER(Expr, isMacro) { return Node.getExprLoc().isMacroID(); } +static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, const Expr *&RhsExpr) { + if (!LhsExpr || !RhsExpr) + return false; -AST_MATCHER_P(Expr, expandedByMacro, std::set<std::string>, Names) { - const SourceManager &SM = Finder->getASTContext().getSourceManager(); - const LangOptions &LO = Finder->getASTContext().getLangOpts(); - 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; + SourceLocation LhsLoc = LhsExpr->getExprLoc(); + SourceLocation RhsLoc = RhsExpr->getExprLoc(); + + return LhsLoc.isMacroID() != RhsLoc.isMacroID(); } void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) { const auto AnyLiteralExpr = ignoringParenImpCasts( anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral())); - std::vector<std::string> MacroNames = - utils::options::parseStringList(KnownBannedMacroNames); - std::set<std::string> Names(MacroNames.begin(), MacroNames.end()); - - const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(Names)); + const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(KnownBannedMacroNames)); + // Binary with equivalent operands, like (X != 2 && X != 2). Finder->addMatcher( binaryOperator(anyOf(hasOperatorName("-"), hasOperatorName("/"), hasOperatorName("%"), hasOperatorName("|"), @@ -499,15 +583,16 @@ .bind("binary"), this); + // Conditional (trenary) operator with equivalent operands, like (Y ? X : X). Finder->addMatcher( conditionalOperator(expressionsAreEquivalent(), // Filter noisy false positives. unless(conditionalOperatorIsInMacro()), - unless(hasTrueExpression(AnyLiteralExpr)), unless(isInTemplateInstantiation())) .bind("cond"), this); + // Overloaded operators with equivalent operands. Finder->addMatcher( cxxOperatorCallExpr( anyOf( @@ -613,8 +698,8 @@ !areEquivalentExpr(LhsSymbol, RhsSymbol)) return; - canonicalNegateExpr(LhsOpcode, LhsValue); - canonicalNegateExpr(RhsOpcode, RhsValue); + transformSubToCanonicalAddExpr(LhsOpcode, LhsValue); + transformSubToCanonicalAddExpr(RhsOpcode, RhsValue); // Check expressions: x + 1 == x + 2 or x + 1 != x + 2. if (LhsOpcode == BO_Add && RhsOpcode == BO_Add) { @@ -674,31 +759,39 @@ if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>( "comparisons-of-symbol-and-const")) { // Matched expressions are: (x <op> k1) <REL> (x <op> k2). + // E.g.: (X < 2) && (X > 4) BinaryOperatorKind Opcode = ComparisonOperator->getOpcode(); const Expr *LhsExpr = nullptr, *RhsExpr = nullptr; - APSInt LhsValue, RhsValue; const Expr *LhsSymbol = nullptr, *RhsSymbol = nullptr; + const Expr *LhsConst = nullptr, *RhsConst = nullptr; BinaryOperatorKind LhsOpcode, RhsOpcode; + APSInt LhsValue, RhsValue; + if (!retrieveRelationalIntegerConstantExpr( - Result, "lhs", LhsExpr, LhsOpcode, LhsSymbol, LhsValue) || + Result, "lhs", LhsExpr, LhsOpcode, LhsSymbol, LhsValue, LhsConst) || !retrieveRelationalIntegerConstantExpr( - Result, "rhs", RhsExpr, RhsOpcode, RhsSymbol, RhsValue) || + Result, "rhs", RhsExpr, RhsOpcode, RhsSymbol, RhsValue, RhsConst) || !areEquivalentExpr(LhsSymbol, RhsSymbol)) return; - // Bring to a canonical form: smallest constant must be on the left side. + // Bring expr to a canonical form: smallest constant must be on the left. if (APSInt::compareValues(LhsValue, RhsValue) > 0) { std::swap(LhsExpr, RhsExpr); std::swap(LhsValue, RhsValue); std::swap(LhsSymbol, RhsSymbol); std::swap(LhsOpcode, RhsOpcode); } + // Constants come from two different macros, or one of them is a macro. + if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) || + areExprsMacroAndNonMacro(LhsConst, RhsConst)) + return; + if ((Opcode == BO_LAnd || Opcode == BO_LOr) && areEquivalentRanges(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) { diag(ComparisonOperator->getOperatorLoc(), - "equivalent expression on both side of logical operator"); + "equivalent expression on both sides of logical operator"); return; } @@ -727,16 +820,62 @@ } void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) { - if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binary")) - diag(BinOp->getOperatorLoc(), "both side of operator are equivalent"); - if (const auto *CondOp = Result.Nodes.getNodeAs<ConditionalOperator>("cond")) - diag(CondOp->getColonLoc(), "'true' and 'false' expression are equivalent"); - if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call")) + 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 (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) || + areExprsMacroAndNonMacro(LhsConst, RhsConst)) + return; + } + + diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent"); + } + + if (const auto *CondOp = + Result.Nodes.getNodeAs<ConditionalOperator>("cond")) { + const Expr *TrueExpr = CondOp->getTrueExpr(); + const Expr *FalseExpr = CondOp->getFalseExpr(); + + if (areExprsFromDifferentMacros(TrueExpr, FalseExpr, Result.Context) || + areExprsMacroAndNonMacro(TrueExpr, FalseExpr)) + return; + diag(CondOp->getColonLoc(), + "'true' and 'false' expressions are equivalent"); + } + + if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call")) { diag(Call->getOperatorLoc(), - "both side of overloaded operator are equivalent"); + "both sides of overloaded operator are equivalent"); + } + // Check for the following bound expressions: + // - "binop-const-compare-to-sym", + // - "binop-const-compare-to-binop-const", + // Produced message: + // -> "logical expression is always false/true" checkArithmeticExpr(Result); + + // Check for the following bound expression: + // - "binop-const-compare-to-const", + // Produced message: + // -> "logical expression is always false/true" checkBitwiseExpr(Result); + + // Check for te following bound expression: + // - "comparisons-of-symbol-and-const", + // Produced messages: + // -> "equivalent expression on both sides of logical operator", + // -> "logical expression is always false/true" + // -> "expression is redundant" checkRelationalExpr(Result); } Index: clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h +++ clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h @@ -16,7 +16,8 @@ namespace tidy { namespace misc { -/// Detect useless or suspicious redundant expressions. +/// The checker detects expressions that are redundant, because they contain +/// ineffective, useless parts. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/misc-redundant-expression.html Index: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst @@ -9,13 +9,13 @@ - redundant, -- always be ``true``, +- always ``true``, -- always be ``false``, +- always ``false``, -- always be a constant (zero or one). +- always a constant (zero or one). -Example: +Examples: .. code-block:: c++
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits