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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

A few nits.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:135
@@ +134,3 @@
+  llvm::APSInt ValueLHS_plus1;
+  if (((OpcodeLHS == BO_LE && OpcodeRHS == BO_LT) ||
+       (OpcodeLHS == BO_GT && OpcodeRHS == BO_GE)) &&
----------------
How about replacing `if (x) return true; return false;` with `return x;`?

BTW, next function looks fine in this regard, since it has a chain of `if (x) return true; if (y) return true; ...`.

================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:182
@@ +181,3 @@
+      incrementWithoutOverflow(ValueLHS, ValueLHS_plus1) &&
+      llvm::APSInt::compareValues(ValueLHS_plus1, ValueRHS) == 0) {
+    return true;
----------------
Remove braces for consistency.

================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:191
@@ +190,3 @@
+// expressions covers the whole domain (i.e. x < 10  and  x > 0).
+static bool doRangesFullyCoverDomain(BinaryOperatorKind OpcodeLHS,
+                                     const llvm::APSInt &ValueLHS,
----------------
I'd slightly prefer it without "do": `rangesFullyCoverDomain`, `rangeSubsumesRange`, etc.

================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:266
@@ +265,3 @@
+  }
+  return false;
+}
----------------
The last return seems to be dead code.

================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:364
@@ +363,3 @@
+
+  // A cast can be matched as a comparator to zero.
+  const auto CastExpr =
----------------
Not sure I understand this comment.

================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:628
@@ +627,3 @@
+
+    llvm::APSInt LhsValue, RhsValue;
+    const Expr *LhsSymbol = nullptr;
----------------
`using llvm::APSInt;` to remove some boilerplate?

================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:651
@@ +650,3 @@
+    if (LhsOpcode == BO_Or && (LhsConstant | RhsConstant) != RhsConstant) {
+      if (Opcode == BO_EQ)
+        diag(ComparisonOperator->getOperatorLoc(),
----------------
Please add braces, when the body is more than one line.

================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:655
@@ +654,3 @@
+      else if (Opcode == BO_NE)
+        diag(ComparisonOperator->getOperatorLoc(),
+             "logical expression is always true");
----------------
`ComparisonOperator->getOperatorLoc()` and the message are repeated too many times. Pull them to local variables / constants?


http://reviews.llvm.org/D21392