barancsuk updated this revision to Diff 119452. barancsuk marked 8 inline comments as done. barancsuk added a comment. Herald added a subscriber: whisperity.
Address review comments. https://reviews.llvm.org/D38688 Files: clang-tidy/misc/RedundantExpressionCheck.cpp clang-tidy/misc/RedundantExpressionCheck.h docs/clang-tidy/checks/misc-redundant-expression.rst test/clang-tidy/misc-redundant-expression.cpp
Index: test/clang-tidy/misc-redundant-expression.cpp =================================================================== --- test/clang-tidy/misc-redundant-expression.cpp +++ 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,16 +386,23 @@ 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 + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: expression is redundant if (X == 11 && X > 10) return 1; // CHECK-MESSAGES: :[[@LINE-1]]:20: 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,124 @@ 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 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_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_OTHER_MACRO) return 1; + + if (X < SOME_OTHER_MACRO || X >= SOME_MACRO) return 1; + if (X <= SOME_OTHER_MACRO || X > SOME_MACRO) return 1; + if (X < SOME_OTHER_MACRO || X > SOME_MACRO) return 1; + if (X <= SOME_OTHER_MACRO || X >= SOME_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; + + // 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 > 9) 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; + + if (X < 9 || X >= SOME_MACRO) return 1; + if (X <= 9 || X > SOME_MACRO) return 1; + if (X < 9 || X > SOME_MACRO) return 1; + if (X <= 9 || X >= SOME_MACRO) return 1; + + if (X == SOME_MACRO || X == 3) return 1; + if (X == SOME_MACRO || X <= 3) return 1; + if (X == SOME_MACRO || X > 3) return 1; + + // Match for macro expressions that are redundant, regardless of the macros` actual + // value. + // Test cases for two different macros. + if (X < SOME_MACRO && X <= SOME_OTHER_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: expression is redundant + if (X >= SOME_MACRO && X > SOME_OTHER_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X == SOME_MACRO && X > SOME_OTHER_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always false + if (X == SOME_MACRO && X != SOME_OTHER_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: expression is redundant + if (X == SOME_MACRO && X != SOME_MACRO_SAME_VALUE) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always false + if (X == SOME_MACRO && X == SOME_OTHER_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always false + if (X == SOME_MACRO_SAME_VALUE && X == SOME_MACRO ) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: both sides of operator are equivalent + + if (X < SOME_MACRO || X <= SOME_OTHER_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X >= SOME_MACRO || X > SOME_OTHER_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: expression is redundant + if (X == SOME_MACRO || X != SOME_OTHER_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X == SOME_MACRO || X != SOME_MACRO_SAME_VALUE) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always true + if (X != SOME_MACRO || X != SOME_OTHER_MACRO) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always true + if (X != SOME_MACRO || X != SOME_MACRO_SAME_VALUE) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: both sides of operator are equivalent + + // Test cases for a macro and a const. + if (X < SOME_MACRO && X <= 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: expression is redundant + if (X >= SOME_MACRO && X > 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X == SOME_MACRO && X > 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always false + if (X == SOME_MACRO && X != 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: expression is redundant + if (X == SOME_MACRO && X == 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always false + + if (X < SOME_MACRO || X <= 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X >= SOME_MACRO || X > 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: expression is redundant + if (X == SOME_MACRO || X != 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant + if (X != SOME_MACRO || X == 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: expression is redundant + if (X != SOME_MACRO || X != 9) return 1; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always true + + // 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 +#undef SOME_OTHER_MACRO +#undef SOME_MACRO_SAME_VALUE +#undef SOME_MACRO return 0; } @@ -419,7 +583,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 +601,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 +614,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 +643,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: docs/clang-tidy/checks/misc-redundant-expression.rst =================================================================== --- docs/clang-tidy/checks/misc-redundant-expression.rst +++ 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++ Index: clang-tidy/misc/RedundantExpressionCheck.h =================================================================== --- clang-tidy/misc/RedundantExpressionCheck.h +++ 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 +/// uneffective, useless parts. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/misc-redundant-expression.html Index: clang-tidy/misc/RedundantExpressionCheck.cpp =================================================================== --- clang-tidy/misc/RedundantExpressionCheck.cpp +++ 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; @@ -87,6 +86,7 @@ case Stmt::CharacterLiteralClass: return cast<CharacterLiteral>(Left)->getValue() == cast<CharacterLiteral>(Right)->getValue(); + case Stmt::IntegerLiteralClass: { llvm::APInt LeftLit = cast<IntegerLiteral>(Left)->getValue(); llvm::APInt RightLit = cast<IntegerLiteral>(Right)->getValue(); @@ -96,6 +96,7 @@ case Stmt::FloatingLiteralClass: return cast<FloatingLiteral>(Left)->getValue().bitwiseIsEqual( cast<FloatingLiteral>(Right)->getValue()); + case Stmt::StringLiteralClass: return cast<StringLiteral>(Left)->getBytes() == cast<StringLiteral>(Right)->getBytes(); @@ -107,16 +108,18 @@ return areEquivalentNameSpecifier( cast<DependentScopeDeclRefExpr>(Left)->getQualifier(), cast<DependentScopeDeclRefExpr>(Right)->getQualifier()); + case Stmt::DeclRefExprClass: return cast<DeclRefExpr>(Left)->getDecl() == cast<DeclRefExpr>(Right)->getDecl(); 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: @@ -128,6 +131,7 @@ return false; return cast<UnaryOperator>(Left)->getOpcode() == cast<UnaryOperator>(Right)->getOpcode(); + case Stmt::BinaryOperatorClass: return cast<BinaryOperator>(Left)->getOpcode() == cast<BinaryOperator>(Right)->getOpcode(); @@ -282,7 +286,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 +300,81 @@ 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()) { + std::string 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 values 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); + + if (ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context)) + return true; + + return false; +} + +// 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(); @@ -331,8 +385,8 @@ return false; } -// Match a binary operator between a symbolic expression and an integer constant -// expression. +// Matches binary operators that are between a symbolic expression and an +// integer constant expression. static ast_matchers::internal::Matcher<Expr> matchBinOpIntegerConstantExpr(StringRef Id) { const auto BinOpCstExpr = @@ -348,7 +402,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 +416,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 +442,7 @@ hasUnaryOperand(anyOf(CastExpr, RelationalExpr))) .bind(NegateId); + // Do not bind to double negation, it is uneffective. const auto NegateNegateRelationalExpr = unaryOperator(hasOperatorName("!"), hasUnaryOperand(unaryOperator( @@ -398,23 +453,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 +487,174 @@ 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) { + if (!isa<BinaryOperator>(BinOp->getLHS()) || + !isa<BinaryOperator>(BinOp->getRHS())) + return false; + + BinaryOperator* LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS()); + BinaryOperator* RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS()); + + if ((LhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) || + LhsBinOp->getRHS()->isIntegerConstantExpr(*AstCtx)) && + (RhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) || + RhsBinOp->getRHS()->isIntegerConstantExpr(*AstCtx))) + return true; + return false; } -AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) { - return areEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr()); +// 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(); + + BinaryOperator *BinOpLhs = dyn_cast<BinaryOperator>(BinOp->getLHS()); + BinaryOperator *BinOpRhs = dyn_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(CallExpr, parametersAreEquivalent) { - return Node.getNumArgs() == 2 && - areEquivalentExpr(Node.getArg(0), Node.getArg(1)); +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(); + + StringRef LhsMacroName = Lexer::getImmediateMacroName(LhsLoc, SM, LO); + StringRef RhsMacroName = Lexer::getImmediateMacroName(RhsLoc, SM, LO); + + if (LhsMacroName == RhsMacroName) + return false; + return true; } -AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) { - return Node.getOperatorLoc().isMacroID(); +static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, const Expr *&RhsExpr) { + if (!LhsExpr || !RhsExpr) + return false; + + SourceLocation LhsLoc = LhsExpr->getExprLoc(); + SourceLocation RhsLoc = RhsExpr->getExprLoc(); + + if ((LhsLoc.isMacroID() && !RhsLoc.isMacroID()) || + (!LhsLoc.isMacroID() && RhsLoc.isMacroID())) + return true; + return false; } -AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) { - return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID(); +static bool isLessOperator(BinaryOperatorKind &Opcode) { + return Opcode == BO_LT || Opcode == BO_LE; } -AST_MATCHER(Expr, isMacro) { return Node.getExprLoc().isMacroID(); } +static bool isGreaterOperator(BinaryOperatorKind &Opcode) { + return Opcode == BO_GT || Opcode == BO_GE; +} -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()) +// Checks whether a set of operators are able to create a meaningful and +// non-redundant expression. +// The function is used by `isIntendedMacroExpression` to filter operator sets +// like (<, &&, <=), that always introduce redundant expressions. +static bool isOperatorSetMeaningful(BinaryOperatorKind &Opcode, + BinaryOperatorKind &LhsOpcode, + BinaryOperatorKind &RhsOpcode) { + + if (!BinaryOperator::isLogicalOp(Opcode)) + return false; + + if ((isLessOperator(LhsOpcode) && isGreaterOperator(RhsOpcode)) || + (isLessOperator(RhsOpcode) && isGreaterOperator(LhsOpcode))) + return true; + + if (Opcode == BO_LAnd) { + if (LhsOpcode == BO_NE && + (RhsOpcode == BO_NE || BinaryOperator::isRelationalOp(RhsOpcode))) return true; - Loc = SM.getImmediateMacroCallerLoc(Loc); + if (RhsOpcode == BO_NE && BinaryOperator::isRelationalOp(LhsOpcode)) + return true; + return false; } + + if (Opcode == BO_LOr) { + if (LhsOpcode == BO_EQ && + (RhsOpcode == BO_EQ || BinaryOperator::isRelationalOp(RhsOpcode))) + return true; + if (RhsOpcode == BO_EQ && BinaryOperator::isRelationalOp(LhsOpcode)) + return true; + return false; + } + + return false; +} + +// Checks whether an expression containing one or two macros, +// like: ((Expr <op> M1/K) <REL> (Expr <op> M2)), can be a meaningful one +// based on the operators. +// A macro expression is considered meaningful, if the macro or macros can hold +// values, with which the expression cannot be simplified further. +// The decison is independent from the actual values of the macros, since +// they may vary across builds. +// Examples: +// The function discards (X < M1 && X <= M2), because it can always be +// simplified to X < M, where M is the smaller one of the two macro +// values. +// An expresion that can be considered meaningful and intended is: +// (X < M1 && X > M2), even when the two ranges are exclusive. +static bool isIntendedMacroExpression(BinaryOperatorKind &Opcode, + BinaryOperatorKind &LhsOpcode, + const Expr *&LhsExpr, + BinaryOperatorKind &RhsOpcode, + const Expr *&RhsExpr, + const ASTContext *AstCtx) { + if ((areExprsFromDifferentMacros(LhsExpr, RhsExpr, AstCtx) || + areExprsMacroAndNonMacro(LhsExpr, RhsExpr)) && + isOperatorSetMeaningful(Opcode, LhsOpcode, RhsOpcode)) + return true; return false; } 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 +673,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 +788,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 +849,38 @@ 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); } + if (isIntendedMacroExpression(Opcode, LhsOpcode, LhsConst, RhsOpcode, + RhsConst, Result.Context)) + 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 +909,63 @@ } 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 (isIntendedMacroExpression(MainOpcode, SideOpcode, LhsConst, + SideOpcode, RhsConst, Result.Context)) + return; + } + + diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent"); + } + if (const auto *CondOp = + Result.Nodes.getNodeAs<ConditionalOperator>("cond")) { + + const auto *TrueExpr = CondOp->getTrueExpr(); + const auto *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 binded 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 binded expression: + // - "binop-const-compare-to-const", + // Produced message: + // -> "logical expression is always false/true" checkBitwiseExpr(Result); + + // Check for te following binded 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); }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits