[PATCH] D21392: [clang-tidy] Enhance redundant-expression check

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:27
@@ -25,1 +26,3 @@
 
+static bool incrementWithoutOverflow(const llvm::APSInt &Value,
+                                     llvm::APSInt &Result) {
----------------
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 &R);
+  void checkBitwiseExpr(const ast_matchers::MatchFinder::MatchResult &R);
----------------
Hmmm, good point. Drat!


http://reviews.llvm.org/D21392