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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Thanks for working on this, these additions look very useful to me.



================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:124
 
+  case Stmt::CXXFunctionalCastExprClass:
+    return cast<CXXFunctionalCastExpr>(Left)->getTypeAsWritten() ==
----------------
You could merge this case with the above case if you cast Left and Right to ExplicitCastExpr.  So both label could end up executing the same code. 


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:330
+
+AST_MATCHER_P(Expr, expandedByMacro, std::set<std::string>, Names) {
+  const SourceManager &SM = Finder->getASTContext().getSourceManager();
----------------
`llvm::StringSet` might be a more efficient choice.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:336
+    std::string MacroName = Lexer::getImmediateMacroName(Loc, SM, LO);
+    if (Names.find(MacroName) != Names.end())
+      return true;
----------------
You could use `count` here. 


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:357
+  ConstExpr = Result.Nodes.getNodeAs<Expr>(CstId);
+  return ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context);
+}
----------------
I think you could just return the pointer and return a null pointer in case it is not an integerConstantExpr. This way no compatibility overload needed.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:510
+// E.g.: from (X == 5) && (X == 5) retrieves 5 and 5
+static void retrieveConstExprFromBothSides(const BinaryOperator *&BinOp,
+                                           BinaryOperatorKind &MainOpcode,
----------------
I would prefer to return a pair of pointer to be returned to output parameters. 


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:579
 
-AST_MATCHER_P(Expr, expandedByMacro, std::set<std::string>, Names) {
-  const SourceManager &SM = Finder->getASTContext().getSourceManager();
-  const LangOptions &LO = Finder->getASTContext().getLangOpts();
-  SourceLocation Loc = Node.getExprLoc();
-  while (Loc.isMacroID()) {
-    std::string MacroName = Lexer::getImmediateMacroName(Loc, SM, LO);
-    if (Names.find(MacroName) != Names.end())
+static bool isOperatorSetMeaningful(BinaryOperatorKind &Opcode,
+                                    BinaryOperatorKind &LhsOpcode,
----------------
I think a comment might be good here what do you mean by meaningful.


================
Comment at: test/clang-tidy/misc-redundant-expression.cpp:387
   // Should not match.
-  if (X == 10 && Y == 10) return 1;
-  if (X != 10 && X != 12) return 1;
----------------
Why did you remove these test cases?


================
Comment at: test/clang-tidy/misc-redundant-expression.cpp:404
   if (X <= 10 && X >= 10) return 1;
-  if (!X && !Y) return 1;
-  if (!X && Y) return 1;
----------------
Same comment as above.


Repository:
  rL LLVM

https://reviews.llvm.org/D38688