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