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

[Etienne Bergeron via cfe-commits]

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<Expr> 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 &R);
----------------
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