Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
etienneb updated this revision to Diff 63020.
etienneb marked 2 inline comments as done.
etienneb added a comment.
nits
http://reviews.llvm.org/D21392
Files:
clang-tidy/misc/RedundantExpressionCheck.cpp
clang-tidy/misc/RedundantExpressionCheck.h
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
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s misc-redundant-expression %t
+// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -std=c++11
+
+typedef __INT64_TYPE__ I64;
struct Point {
int x;
@@ -64,7 +66,7 @@
if ( + "dummy" == + "dummy") return 1;
// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
- if (L"abc" == L"abc") return 1;
+ if (L"abc" == L"abc") return 1;
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
if (foo(0) - 2 < foo(0) - 2) return 1;
@@ -82,7 +84,7 @@
int Valid(int X, int Y) {
if (X != Y) return 1;
- if (X == X + 0) return 1;
+ if (X == Y + 0) return 1;
if (P.x == P.y) return 1;
if (P.a[P.x] < P.a[P.y]) return 1;
if (P.a[0] < P.a[1]) return 1;
@@ -160,3 +162,324 @@
// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of overloaded operator are equivalent
}
void TestTemplate() { TemplateCheck(); }
+
+int TestArithmetic(int X, int Y) {
+ if (X + 1 == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+ if (X - 1 == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X - 1 != X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1LL == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+ if (X + 1ULL == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false
+
+ if (X == X + 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+ if (X != X + 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+ if (X == X - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+ if (X != X - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+ if (X != X - 1U) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+ if (X != X - 1LL) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+ if ((X+X) != (X+X) - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X + 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X + 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X - 1 == X - 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X - 1 != X - 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X - -1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+ if (X + 1 != X - -1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 == X - -2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X - -2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X - (~0)) return 1;
+ // 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
+
+ // Should not match.
+ if (X + 0.5 == X) return 1;
+ if (X + 1 == Y) return 1;
+ if (X + 1 == Y + 1) return 1;
+ if (X + 1 == Y + 2) return 1;
+
+ return 0;
+}
+
+int TestBitwise(int X) {
+ if ((X & 0xFF) == 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+ if ((X & 0xFF) != 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+ if ((X | 0xFF) == 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+ if ((X | 0xFF) != 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+
+ if ((X | 0xFFULL)
Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
A couple of nits. Otherwise looks good. Thanks!
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:232
@@ +231,3 @@
+
+static bool rangeSubsumeRange(BinaryOperatorKind OpcodeLHS,
+ const APSInt ,
nit: rangeSubsumesRange (note: subsume -> subsumes)
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:283
@@ +282,3 @@
+matchIntegerConstantExpr(StringRef Id) {
+ std::string CstId = (Id + "-cst").str();
+ return expr(isIntegerConstantExpr()).bind(CstId);
nit: I'd prefer "const" instead of "cst": saving two characters doesn't change
much, I guess, but the loss of clarity is substantial.
http://reviews.llvm.org/D21392
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
etienneb added inline comments.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:27
@@ -25,1 +26,3 @@
+static bool incrementWithoutOverflow(const llvm::APSInt ,
+ llvm::APSInt ) {
aaron.ballman wrote:
> I think this could be implemented using APInt overflow checks, no?
> `APInt::sadd_ov()`?
It could be implemented using sadd_ov and uadd_ov.
The 'signedness' need to be take into account. The class 'APInt' doesn't not
carry signedness.
Using the operator++ here let the instantiated type do the right increment and
right comparison.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:274
@@ +273,3 @@
+Value = -Value;
+ }
+}
aaron.ballman wrote:
> > In this case -128 (8-bits) will give -128.
>
> So negating -128 doesn't yield 128, it yields -128? That seems weird.
>
> > The APSInt is behaving the same way than a real value of the same width and
> > signedness.
>
> A real value of the same width and signedness has UB with that case, which is
> why I was asking. The range of an 8-bit signed int is -128 to 127, so
> negating -128 yields an out-of-range value. I want to make sure we aren't
> butchering that by canonicalizing the negate expression.
The value produced by 'canonicalNegateExpr' is the same value produced by
executing the sub instruction on the CPU.
Even if the value make no sense in math.
Btw, there is a unittest to cover this case.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:364
@@ +363,3 @@
+
+ // A cast can be matched as a comparator to zero.
+ const auto CastExpr =
alexfh wrote:
> Not sure I understand this comment.
if ( implicit-int-to-bool(x) ) <<-- the implicit-int-to-bool(...) could be
consider as x != 0
http://reviews.llvm.org/D21392
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
etienneb updated this revision to Diff 61984.
etienneb marked 12 inline comments as done.
etienneb added a comment.
address comments
http://reviews.llvm.org/D21392
Files:
clang-tidy/misc/RedundantExpressionCheck.cpp
clang-tidy/misc/RedundantExpressionCheck.h
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
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s misc-redundant-expression %t
+// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -std=c++11
+
+typedef __INT64_TYPE__ I64;
struct Point {
int x;
@@ -64,7 +66,7 @@
if ( + "dummy" == + "dummy") return 1;
// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
- if (L"abc" == L"abc") return 1;
+ if (L"abc" == L"abc") return 1;
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
if (foo(0) - 2 < foo(0) - 2) return 1;
@@ -82,7 +84,7 @@
int Valid(int X, int Y) {
if (X != Y) return 1;
- if (X == X + 0) return 1;
+ if (X == Y + 0) return 1;
if (P.x == P.y) return 1;
if (P.a[P.x] < P.a[P.y]) return 1;
if (P.a[0] < P.a[1]) return 1;
@@ -160,3 +162,324 @@
// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of overloaded operator are equivalent
}
void TestTemplate() { TemplateCheck(); }
+
+int TestArithmetic(int X, int Y) {
+ if (X + 1 == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+ if (X - 1 == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X - 1 != X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1LL == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+ if (X + 1ULL == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false
+
+ if (X == X + 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+ if (X != X + 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+ if (X == X - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+ if (X != X - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+ if (X != X - 1U) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+ if (X != X - 1LL) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+ if ((X+X) != (X+X) - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X + 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X + 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X - 1 == X - 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X - 1 != X - 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X - -1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+ if (X + 1 != X - -1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 == X - -2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X - -2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X - (~0)) return 1;
+ // 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
+
+ // Should not match.
+ if (X + 0.5 == X) return 1;
+ if (X + 1 == Y) return 1;
+ if (X + 1 == Y + 1) return 1;
+ if (X + 1 == Y + 2) return 1;
+
+ return 0;
+}
+
+int TestBitwise(int X) {
+ if ((X & 0xFF) == 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+ if ((X & 0xFF) != 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+ if ((X | 0xFF) == 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+ if ((X | 0xFF) != 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+
+ if
Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
alexfh added a comment.
A few nits.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:135
@@ +134,3 @@
+ llvm::APSInt ValueLHS_plus1;
+ if (((OpcodeLHS == BO_LE && OpcodeRHS == BO_LT) ||
+ (OpcodeLHS == BO_GT && OpcodeRHS == BO_GE)) &&
How about replacing `if (x) return true; return false;` with `return x;`?
BTW, next function looks fine in this regard, since it has a chain of `if (x)
return true; if (y) return true; ...`.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:182
@@ +181,3 @@
+ incrementWithoutOverflow(ValueLHS, ValueLHS_plus1) &&
+ llvm::APSInt::compareValues(ValueLHS_plus1, ValueRHS) == 0) {
+return true;
Remove braces for consistency.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:191
@@ +190,3 @@
+// expressions covers the whole domain (i.e. x < 10 and x > 0).
+static bool doRangesFullyCoverDomain(BinaryOperatorKind OpcodeLHS,
+ const llvm::APSInt ,
I'd slightly prefer it without "do": `rangesFullyCoverDomain`,
`rangeSubsumesRange`, etc.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:266
@@ +265,3 @@
+ }
+ return false;
+}
The last return seems to be dead code.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:364
@@ +363,3 @@
+
+ // A cast can be matched as a comparator to zero.
+ const auto CastExpr =
Not sure I understand this comment.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:628
@@ +627,3 @@
+
+llvm::APSInt LhsValue, RhsValue;
+const Expr *LhsSymbol = nullptr;
`using llvm::APSInt;` to remove some boilerplate?
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:651
@@ +650,3 @@
+if (LhsOpcode == BO_Or && (LhsConstant | RhsConstant) != RhsConstant) {
+ if (Opcode == BO_EQ)
+diag(ComparisonOperator->getOperatorLoc(),
Please add braces, when the body is more than one line.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:655
@@ +654,3 @@
+ else if (Opcode == BO_NE)
+diag(ComparisonOperator->getOperatorLoc(),
+ "logical expression is always true");
`ComparisonOperator->getOperatorLoc()` and the message are repeated too many
times. Pull them to local variables / constants?
http://reviews.llvm.org/D21392
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
aaron.ballman added inline comments.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:27
@@ -25,1 +26,3 @@
+static bool incrementWithoutOverflow(const llvm::APSInt ,
+ llvm::APSInt ) {
I think this could be implemented using APInt overflow checks, no?
`APInt::sadd_ov()`?
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:138
@@ +137,3 @@
+ incrementWithoutOverflow(ValueLHS, ValueLHS_plus1) &&
+ llvm::APSInt::compareValues(ValueLHS_plus1, ValueRHS) == 0)
+return true;
Ah, this may be confusion on my part. I was thinking "equivalent ranges"
meaning "does one range cover another range", e.g., `x < 12` is also covered by
`x < 4` in an expression like `if (x < 4 && x < 12)`.
I think this code is fine as-is.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:274
@@ +273,3 @@
+Value = -Value;
+ }
+}
> In this case -128 (8-bits) will give -128.
So negating -128 doesn't yield 128, it yields -128? That seems weird.
> The APSInt is behaving the same way than a real value of the same width and
> signedness.
A real value of the same width and signedness has UB with that case, which is
why I was asking. The range of an 8-bit signed int is -128 to 127, so negating
-128 yields an out-of-range value. I want to make sure we aren't butchering
that by canonicalizing the negate expression.
Comment at: clang-tidy/misc/RedundantExpressionCheck.h:31
@@ +30,3 @@
+private:
+ void checkArithmeticExpr(const ast_matchers::MatchFinder::MatchResult );
+ void checkBitwiseExpr(const ast_matchers::MatchFinder::MatchResult );
Hmmm, good point. Drat!
http://reviews.llvm.org/D21392
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
etienneb updated this revision to Diff 60983.
etienneb added a comment.
refactoring
http://reviews.llvm.org/D21392
Files:
clang-tidy/misc/RedundantExpressionCheck.cpp
clang-tidy/misc/RedundantExpressionCheck.h
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
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s misc-redundant-expression %t
+// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -std=c++11
+
+typedef __INT64_TYPE__ I64;
struct Point {
int x;
@@ -64,7 +66,7 @@
if ( + "dummy" == + "dummy") return 1;
// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
- if (L"abc" == L"abc") return 1;
+ if (L"abc" == L"abc") return 1;
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
if (foo(0) - 2 < foo(0) - 2) return 1;
@@ -82,7 +84,7 @@
int Valid(int X, int Y) {
if (X != Y) return 1;
- if (X == X + 0) return 1;
+ if (X == Y + 0) return 1;
if (P.x == P.y) return 1;
if (P.a[P.x] < P.a[P.y]) return 1;
if (P.a[0] < P.a[1]) return 1;
@@ -160,3 +162,324 @@
// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of overloaded operator are equivalent
}
void TestTemplate() { TemplateCheck(); }
+
+int TestArithmetic(int X, int Y) {
+ if (X + 1 == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+ if (X - 1 == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X - 1 != X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1LL == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+ if (X + 1ULL == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false
+
+ if (X == X + 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+ if (X != X + 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+ if (X == X - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+ if (X != X - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+ if (X != X - 1U) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+ if (X != X - 1LL) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+ if ((X+X) != (X+X) - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X + 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X + 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X - 1 == X - 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X - 1 != X - 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X - -1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+ if (X + 1 != X - -1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 == X - -2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X - -2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X - (~0)) return 1;
+ // 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
+
+ // Should not match.
+ if (X + 0.5 == X) return 1;
+ if (X + 1 == Y) return 1;
+ if (X + 1 == Y + 1) return 1;
+ if (X + 1 == Y + 2) return 1;
+
+ return 0;
+}
+
+int TestBitwise(int X) {
+ if ((X & 0xFF) == 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+ if ((X & 0xFF) != 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+ if ((X | 0xFF) == 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+ if ((X | 0xFF) != 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+
+ if ((X | 0xFFULL) != 0xF00) return 1;
+ //
Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
etienneb added a comment.
thx Aaron.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:131
@@ +130,3 @@
+
+ // Handle the case where constants are off by one: x <= 4 <==> x < 5.
+ llvm::APSInt ValueLHS_plus1 = ValueLHS;
aaron.ballman wrote:
> Why is off-by-one more useful than a stronger range analysis?
I don't get your point?
This function is comparing two ranges. Ranges are comparable only if they are
compared to the same constant
OR, if they are off-by one and the operator contains "equality".
As the comment state, x <= 4 is equivalent to x < 5.
To avoid problem with wrap around, the left value is incremented (the left
value must be the smaller one: canonical form).
I don't get why we should use a range analysis? Is there some utility functions
I'm not aware of?
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:177
@@ +176,3 @@
+ // Handle cases where the constants are different.
+ if ((OpcodeLHS == BO_EQ || OpcodeLHS == BO_LE || OpcodeLHS == BO_LE) &&
+ (OpcodeRHS == BO_EQ || OpcodeRHS == BO_GT || OpcodeRHS == BO_GE))
aaron.ballman wrote:
> Can this be done before doing the more expensive value comparisons?
it can be done before the previous if. But not the one at line 149 (which
always returns).
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:267
@@ +266,3 @@
+Opcode = BO_Add;
+Value = -Value;
+ }
aaron.ballman wrote:
> I can never remember myself, but how well does APSInt handle this situation
> if it causes overflow of the signed value? e.g., an 8-bit APSInt holding the
> value -128 being negated to 128 (which is outside the range of an 8-bit
> signed integer).
In this case -128 (8-bits) will give -128.
The APSInt is behaving the same way than a real value of the same width and
signedness.
I added an unittest to cover this case.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:294
@@ +293,3 @@
+// Returns a matcher for a generic expression (not a constant expression).
+static ast_matchers::internal::Matcher matchGenericExpr(StringRef Id) {
+ std::string SymId = (Id + "-gen").str();
aaron.ballman wrote:
> I'm not keen on this name because C11 has `_Generic`, for which we have a
> `GenericSelectionExpr` which is awfully close to this name.
'Generic' is a too generic name :)
Comment at: clang-tidy/misc/RedundantExpressionCheck.h:31
@@ +30,3 @@
+private:
+ void checkArithmeticExpr(const ast_matchers::MatchFinder::MatchResult & R);
+ void checkBitwiseExpr(const ast_matchers::MatchFinder::MatchResult );
aaron.ballman wrote:
> `&` should bind to `R`.
>
> Also, all of these functions can be marked `const`.
They can't be constant, they are calling 'diag' which is not const.
```
error: no matching function for call to
‘clang::tidy::misc::RedundantExpressionCheck::diag(clang::SourceLocation, const
char [35]) const
```
http://reviews.llvm.org/D21392
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
etienneb updated this revision to Diff 60982.
etienneb marked 7 inline comments as done.
etienneb added a comment.
address comments
http://reviews.llvm.org/D21392
Files:
clang-tidy/misc/RedundantExpressionCheck.cpp
clang-tidy/misc/RedundantExpressionCheck.h
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
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s misc-redundant-expression %t
+// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -std=c++11
+
+typedef __INT64_TYPE__ I64;
struct Point {
int x;
@@ -64,7 +66,7 @@
if ( + "dummy" == + "dummy") return 1;
// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
- if (L"abc" == L"abc") return 1;
+ if (L"abc" == L"abc") return 1;
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
if (foo(0) - 2 < foo(0) - 2) return 1;
@@ -82,7 +84,7 @@
int Valid(int X, int Y) {
if (X != Y) return 1;
- if (X == X + 0) return 1;
+ if (X == Y + 0) return 1;
if (P.x == P.y) return 1;
if (P.a[P.x] < P.a[P.y]) return 1;
if (P.a[0] < P.a[1]) return 1;
@@ -160,3 +162,324 @@
// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of overloaded operator are equivalent
}
void TestTemplate() { TemplateCheck(); }
+
+int TestArithmetic(int X, int Y) {
+ if (X + 1 == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+ if (X - 1 == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X - 1 != X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1LL == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+ if (X + 1ULL == X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false
+
+ if (X == X + 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+ if (X != X + 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+ if (X == X - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+ if (X != X - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+ if (X != X - 1U) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+ if (X != X - 1LL) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+ if ((X+X) != (X+X) - 1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X + 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X + 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X - 1 == X - 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X - 1 != X - 2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X - -1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+ if (X + 1 != X - -1) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 == X - -2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+ if (X + 1 != X - -2) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+ if (X + 1 == X - (~0)) return 1;
+ // 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
+
+ // Should not match.
+ if (X + 0.5 == X) return 1;
+ if (X + 1 == Y) return 1;
+ if (X + 1 == Y + 1) return 1;
+ if (X + 1 == Y + 2) return 1;
+
+ return 0;
+}
+
+int TestBitwise(int X) {
+ if ((X & 0xFF) == 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+ if ((X & 0xFF) != 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+ if ((X | 0xFF) == 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+ if ((X | 0xFF) != 0xF00) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+
+ if
Re: [PATCH] D21392: [clang-tidy] Enhance redundant-expression check
aaron.ballman added inline comments.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:114
@@ -112,1 +113,3 @@
+// Perform a comparison bitween APSInt with respect to bit-width and
signedness.
+static int compareValues(const llvm::APSInt ,
s/bitween/between, however, I don't think this function adds much value; why
not just call `APSInt::compareValues()` directly?
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:131
@@ +130,3 @@
+
+ // Handle the case where constants are off by one: x <= 4 <==> x < 5.
+ llvm::APSInt ValueLHS_plus1 = ValueLHS;
Why is off-by-one more useful than a stronger range analysis?
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:171
@@ +170,3 @@
+ ++ValueLHS_plus1;
+ if (compareValues(ValueLHS_plus1, ValueRHS) == 0 && OpcodeLHS == BO_GT &&
+ OpcodeRHS == BO_LT) {
Same question here as above.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:177
@@ +176,3 @@
+ // Handle cases where the constants are different.
+ if ((OpcodeLHS == BO_EQ || OpcodeLHS == BO_LE || OpcodeLHS == BO_LE) &&
+ (OpcodeRHS == BO_EQ || OpcodeRHS == BO_GT || OpcodeRHS == BO_GE))
Can this be done before doing the more expensive value comparisons?
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:227
@@ +226,3 @@
+
+static bool doRangeSubsumeRange(BinaryOperatorKind OpcodeLHS,
+const llvm::APSInt ,
s/do/does
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:267
@@ +266,3 @@
+Opcode = BO_Add;
+Value = -Value;
+ }
I can never remember myself, but how well does APSInt handle this situation if
it causes overflow of the signed value? e.g., an 8-bit APSInt holding the value
-128 being negated to 128 (which is outside the range of an 8-bit signed
integer).
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:294
@@ +293,3 @@
+// Returns a matcher for a generic expression (not a constant expression).
+static ast_matchers::internal::Matcher matchGenericExpr(StringRef Id) {
+ std::string SymId = (Id + "-gen").str();
I'm not keen on this name because C11 has `_Generic`, for which we have a
`GenericSelectionExpr` which is awfully close to this name.
Comment at: clang-tidy/misc/RedundantExpressionCheck.h:31
@@ +30,3 @@
+private:
+ void checkArithmeticExpr(const ast_matchers::MatchFinder::MatchResult & R);
+ void checkBitwiseExpr(const ast_matchers::MatchFinder::MatchResult );
`&` should bind to `R`.
Also, all of these functions can be marked `const`.
http://reviews.llvm.org/D21392
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
