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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Hi :)

I added my thoughts for the check. Many variables in your code could be `const`, i didn't mention all cases.



================
Comment at: clang-tidy/readability/UselessIntermediateVarCheck.cpp:24
+
+  auto directDeclRefExprLHS1 =
+    // We match a direct declaration reference expression pointing
----------------
The matching expression can be `const auto`.
I would write the comment above the declaration, that the splitting does not occur (here and the next simpler matchers).

The local variables for matcher should have a capital letter as start -> s/directDeclRefExprLHS1/DirectDeclRefExpr/ here and elsewhere.


================
Comment at: clang-tidy/readability/UselessIntermediateVarCheck.cpp:62
+
+  auto hasVarDecl1 =
+    // We match a single declaration which is a variable declaration,
----------------
here and elsewhere, is the indendation result of clang-format? Personally i like it, since it shows the structure, but there might be concerns since it probably does not match with the coding guideline.


================
Comment at: clang-tidy/readability/UselessIntermediateVarCheck.cpp:186
+    const VarDecl *VarDecl1, const VarDecl *VarDecl2) {
+  diag(VarDecl1->getLocation(), "intermediate variable %0 is useless",
+       DiagnosticIDs::Warning)
----------------
The warning message sounds a little harsh/judging, and does not explain why the intermediate is useless. (the notes do which is ok i think).

Maybe go with `unnecessary intermediate variable %0`


================
Comment at: clang-tidy/readability/UselessIntermediateVarCheck.cpp:236
+  if (BinOpToReverse) {
+    auto ReversedBinOpText =
+      BinaryOperator::getOpcodeStr(
----------------
`const auto` ?


================
Comment at: clang-tidy/readability/UselessIntermediateVarCheck.cpp:290
+    // declaration.
+    auto Init1TextOpt =
+      utils::lexer::getStmtText(Init1, Result.Context->getSourceManager());
----------------
some `const` is possible for this an the following variables.


================
Comment at: clang-tidy/readability/UselessIntermediateVarCheck.cpp:335
+      // operands of the binary operator to keep the same execution order.
+      auto LHSTextOpt =
+        utils::lexer::getStmtText(BinOp->getLHS(), Result.Context->getSourceManager());
----------------
`const auto`?


================
Comment at: clang-tidy/readability/UselessIntermediateVarCheck.cpp:350
+    // statement.
+    bool HasVarRef1 = VarRefLHS1 || VarRefRHS1;
+    bool HasVarRef2 = VarRefLHS2 || VarRefRHS2;
----------------
`const`


================
Comment at: clang-tidy/readability/UselessIntermediateVarCheck.h:29
+      : ClangTidyCheck(Name, Context),
+        MaximumLineLength(Options.get("MaximumLineLength", 100)) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
----------------
It would be nice, if there is a way to get this information from a clang-format file. 


================
Comment at: clang-tidy/readability/UselessIntermediateVarCheck.h:47
+  unsigned MaximumLineLength;
+  std::unordered_set<const DeclStmt*> CheckedDeclStmt;
+};
----------------
Maybe a SmallSet? the optimized datastructures from llvm would save dynamic memory allocations.


================
Comment at: clang-tidy/utils/Matchers.h:67
+  // We build a Control Flow Graph (CFG) from the parent statement.
+  std::unique_ptr<CFG> StatementCFG
+  = CFG::buildCFG(nullptr, const_cast<Stmt*>(Parent), &Finder->getASTContext(),
----------------
formatted? usually the `=` ends up on the same line


================
Comment at: docs/ReleaseNotes.rst:63
+
+  This new checker detects useless intermediate variables before return
+  statements that return the result of a simple comparison. This checker also
----------------
maybe "useless" should be replaced with "unnecessary". but thats only my opinion.


================
Comment at: docs/clang-tidy/checks/readability-useless-intermediate-var.rst:14
+
+  auto test = 1;
+  return (test == 2);
----------------
especially to avoid magic numbers, this kind of code could be more readable.


================
Comment at: test/clang-tidy/readability-useless-intermediate-var.cpp:1
+// RUN: %check_clang_tidy %s readability-useless-intermediate-var %t
+
----------------
the tests seem to be to less compared to the code volume of the check.

situations i think should be tested:

- initialization from a function, method call
- initialization with a lambda
```
const auto SomeVar = []() { /* super complicated stuff */ return Result; } ();
return SomeVar;
```
- template stuff -> proof that they work two, even thought it doesnt seem to be relevant, at least what i can see.
- what happens if a "temporary" is used as an argument for a function call?
```
const auto Var = std::sqrt(10);
return std::pow(Var, 10);
```
- proof that really long function calls (like STL Algorithm tends to be) are not inlined as well


https://reviews.llvm.org/D37014