[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:114
@@ -112,1 +113,3 @@
 
+// Perform a comparison bitween APSInt with respect to bit-width and signedness.
+static int compareValues(const llvm::APSInt &ValueLHS,
----------------
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 &ValueLHS,
----------------
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<Expr> 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 &R);
----------------
`&` should bind to `R`.

Also, all of these functions can be marked `const`.


http://reviews.llvm.org/D21392