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

[Etienne Bergeron via cfe-commits]

etienneb 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) {
----------------
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