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

Etienne Bergeron via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 28 15:14:08 PDT 2016

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


More information about the cfe-commits mailing list