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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
