[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:89
            cast<CharacterLiteral>(Right)->getValue();
+
   case Stmt::IntegerLiteralClass: {
----------------
I would remove the formatting changes from this function.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:331
+  while (Loc.isMacroID()) {
+    std::string MacroName = Lexer::getImmediateMacroName(Loc, SM, LO);
+    if (Names.count(MacroName))
----------------
This should use a `StringRef` instead of a `std::string` to avoid needless copies.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:347
+// Retrieves the integer expression matched by 'matchIntegerConstantExpr' with
+// name 'Id' and stores it into 'ConstExpr', the values of the expression is
+// stored into `Value`.
----------------
the values -> the value


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:355-358
+  if (ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context))
+    return true;
+
+  return false;
----------------
This can be reduced to `return ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context);`


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:364
+                                        StringRef Id, APSInt &Value) {
+  const Expr* ConstExpr = nullptr;;
+  return retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr);
----------------
The `*` binds to the identifier, and there's an extra `;`.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:388
 
-// Match a binary operator between a symbolic expression and an integer constant
-// expression.
+// Matches binary operators that are between a symbolic expression and an
+// integer constant expression.
----------------
I'd go back towards the original formulation: Match a binary operator between a symbolic expression and an integer constant expression.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:445
 
+  // Do not bind to double negation, it is uneffective.
   const auto NegateNegateRelationalExpr =
----------------
I understand the first part of the comment, but not the second part -- why is double negation ineffective?

(Btw, typo: unaffective -> ineffective)


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:499-500
+
+  BinaryOperator* LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
+  BinaryOperator* RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());
+
----------------
Formatting. Also, this can use `const auto *` instead of spelling the name twice. Actually, this should be reformulated because you do an `isa<>` check above. You can replace the `if` statement and these assignments with:
```
const auto *LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
const auto *RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());

if (!LhsBinOp || !RhsBinOp)
  return false
```


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:524-525
+
+  BinaryOperator *BinOpLhs = dyn_cast<BinaryOperator>(BinOp->getLHS());
+  BinaryOperator *BinOpRhs = dyn_cast<BinaryOperator>(BinOp->getRHS());
+
----------------
These should use `cast<>` instead of `dyn_cast<>` (you've already asserted this property and `cast<>` will assert anyway).


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:537
+
+  assert((BinOpLhs->getOpcode() == BinOpRhs->getOpcode()) &&
+         "Sides of the binary operator must be equivalent expressions!");
----------------
Spurious parens.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:560-565
+  StringRef LhsMacroName = Lexer::getImmediateMacroName(LhsLoc, SM, LO);
+  StringRef RhsMacroName = Lexer::getImmediateMacroName(RhsLoc, SM, LO);
+
+  if (LhsMacroName == RhsMacroName)
+    return false;
+  return true;
----------------
This can be combined into a single `return`  statement and no local variables.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:575-578
+  if ((LhsLoc.isMacroID() && !RhsLoc.isMacroID()) ||
+      (!LhsLoc.isMacroID() && RhsLoc.isMacroID()))
+    return true;
+  return false;
----------------
This can be `return LhsLoc.isMacroID() != RhsLoc.isMacroID();`


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:633-635
+// The function discards  (X < M1 && X <= M2), because it can always be
+// simplified to X < M, where M is the smaller one of the two macro
+// values.
----------------
This worries me slightly for macros because those values can be overridden for different compilation targets. So the diagnostic may be correct for a particular configuration of the macros, but incorrect for another. Have you tested this against any large code bases to see what the quality of the diagnostics looks like?


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:917
+    if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
+
+      const Expr *LhsConst = nullptr, *RhsConst = nullptr;
----------------
Spurious newline.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:934
+          Result.Nodes.getNodeAs<ConditionalOperator>("cond")) {
+
+    const auto *TrueExpr = CondOp->getTrueExpr();
----------------
Spurious newline.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:935-936
+
+    const auto *TrueExpr = CondOp->getTrueExpr();
+    const auto *FalseExpr = CondOp->getFalseExpr();
+
----------------
Please don't use `auto` here as the type is not explicitly spelled out.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:950
 
+  // Check for the following binded expressions:
+  // - "binop-const-compare-to-sym",
----------------
binded? Perhaps you meant bound? (Same comment applies below.)


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.h:20
+/// The checker detects expressions that are redundant, because they contain
+/// uneffective, useless parts.
 ///
----------------
uneffective -> ineffective


https://reviews.llvm.org/D38688