[PATCH] D37014: [clang-tidy] Add a checker to remove useless intermediate variables before return statements with comparisons

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:27-30
+  auto File = Context->getCurrentFile();
+  auto Style = format::getStyle(*Context->getOptionsForFile(File).FormatStyle,
+                                File, "none");
+  auto DefaultMaximumLineLength = 80;
----------------
Please do not use `auto` here as the type is not explicitly spelled out in the initializer. Same comment applies elsewhere.


================
Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:32
+      
+  if (!Style) {
+    DefaultMaximumLineLength = (*Style).ColumnLimit;
----------------
Elide braces.


================
Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:189
+       DiagnosticIDs::Note)
+      << (IsPlural ? "they are" : "it is");
+}
----------------
This should be using a `%select` in the diagnostic.


================
Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:234-241
+  const DeclStmt *DeclarationStmt1 =
+      Result.Nodes.getNodeAs<DeclStmt>("declStmt1");
+  const Expr *Init1 = Result.Nodes.getNodeAs<Expr>("init1");
+  const VarDecl *VariableDecl1 = Result.Nodes.getNodeAs<VarDecl>("varDecl1");
+  const DeclRefExpr *VarRefLHS1 =
+      Result.Nodes.getNodeAs<DeclRefExpr>("declRefExprLHS1");
+  const DeclRefExpr *VarRefRHS1 =
----------------
Can use `const auto *` because the type is explicitly spelled out in the initialization. Same comment applies elsewhere.


================
Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:274
+        clang::tooling::fixit::getText(*Init1, *Result.Context).str();
+    if (Init1Text.empty()) {
+      return;
----------------
Elide braces (applies elsewhere).


================
Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.h:51
+  // all the possible branches of the code and therefore cover all statements.
+  for (auto &Block : *StatementCFG) {
+    if (!Block)
----------------
Can this be `const auto &`?


================
Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.h:57
+    bool ReturnNextStmt = false;
+    for (auto &BlockItem : *Block) {
+      Optional<CFGStmt> CFGStatement = BlockItem.getAs<CFGStmt>();
----------------
Same here.


================
Comment at: docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst:4
+readability-unnecessary-intermediate-var
+====================================
+
----------------
Underline is of incorrect length.


https://reviews.llvm.org/D37014