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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.h:27
+// Matches the next statement within the parent statement sequence.
+AST_MATCHER_P(Stmt, hasSuccessor, ast_matchers::internal::Matcher,
+  InnerMatcher) {

Looking at how this matcher is used, I'm starting to doubt this is an efficient 
way to find the kind of the coding pattern this check targets. Creating CFG for 
each declaration statement would result in a lot of overlapping and, thus, 
unnecessary work. I guess, it should be easy to create a test case (e.g. 
thousands of declarations in the same function that would trigger one or better 
both hasSuccessor matchers) where this check would run for extremely long time. 
I'd recommend doing this to test this or any alternative solution you come up 
with. Performance problems are very real for some clang-tidy checks. And this 
check raises a number of red flags w.r.t. performance.

An alternative I could suggest is to build CFG for each function once and then 
search for the interesting declarations while traversing it (instead of running 
AST matchers and building CFG inside them).


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:1
+//===--- UnnecessaryIntermediateVarCheck.cpp -
+// clang-tidy--===//

Please make fit line into 80 characters and remove incorrect continuation.



Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.h:1
+//===--- UnnecessaryIntermediateVarCheck.h - clang-tidy--*- C++
+//-*-===//

Please make fit line into 80 characters and remove incorrect continuation.



Comment at: docs/ReleaseNotes.rst:128
+- New :doc:`readability-unnecessary-intermediate-var
+  `_ check
+

Final underscore is not needed.



Comment at: 
docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst:6
+
+Detects unnecessary intermediate variables before ``return` statements 
returning the
+result of a simple comparison. This check also suggests to directly inline the

Please add missing `. Same below.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon updated this revision to Diff 140541.
tbourvon marked 3 inline comments as done.
tbourvon added a comment.

Order and link fixes in the release notes


https://reviews.llvm.org/D37014

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
  test/clang-tidy/readability-unnecessary-intermediate-var.cpp
  unittests/clang-tidy/ReadabilityModuleTest.cpp

Index: unittests/clang-tidy/ReadabilityModuleTest.cpp
===
--- unittests/clang-tidy/ReadabilityModuleTest.cpp
+++ unittests/clang-tidy/ReadabilityModuleTest.cpp
@@ -1,6 +1,8 @@
 #include "ClangTidyTest.h"
 #include "readability/BracesAroundStatementsCheck.h"
 #include "readability/NamespaceCommentCheck.h"
+#include "readability/UnnecessaryIntermediateVarCheck.h"
+#include "../../../unittests/ASTMatchers/ASTMatchersTest.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -500,6 +502,19 @@
 runCheckOnCode(Input));
 }
 
+TEST(StatementMatcher, HasSuccessor) {
+  using namespace ast_matchers;
+  using namespace matchers;
+
+  StatementMatcher DeclHasSuccessorReturnStmt =
+  declStmt(hasSuccessor(returnStmt()));
+
+  EXPECT_TRUE(
+  matches("void foo() { int bar; return; }", DeclHasSuccessorReturnStmt));
+  EXPECT_TRUE(notMatches("void foo() { int bar; bar++; return; }",
+ DeclHasSuccessorReturnStmt));
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang
Index: test/clang-tidy/readability-unnecessary-intermediate-var.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-unnecessary-intermediate-var.cpp
@@ -0,0 +1,245 @@
+// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t
+
+bool f() {
+  auto test = 1; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (1 == 1);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f2() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test1 == test2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration
+  // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one
+  // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here
+}
+
+bool f3() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (test1 == 2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f4() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test2 == 3);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f5() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (2 == test1);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate var

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:137
+  
+- New `readability-unnecessary-intermediate-var
+  
`_
 check

Please use alphabetic order and //:doc:// as well as proper link. See other 
checks as example.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon marked an inline comment as done.
tbourvon added inline comments.



Comment at: docs/ReleaseNotes.rst:62
 
 - New `bugprone-throw-keyword-missing
   
`_
 check

Eugene.Zelenko wrote:
> Please rebase from trunk and use //:doc:// for link.
I'm sorry, what do you mean by "use :doc: for link"?


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon updated this revision to Diff 140278.
tbourvon marked 10 inline comments as done.
tbourvon added a comment.

Fixed review comments


https://reviews.llvm.org/D37014

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
  test/clang-tidy/readability-unnecessary-intermediate-var.cpp
  unittests/clang-tidy/ReadabilityModuleTest.cpp

Index: unittests/clang-tidy/ReadabilityModuleTest.cpp
===
--- unittests/clang-tidy/ReadabilityModuleTest.cpp
+++ unittests/clang-tidy/ReadabilityModuleTest.cpp
@@ -1,6 +1,8 @@
 #include "ClangTidyTest.h"
 #include "readability/BracesAroundStatementsCheck.h"
 #include "readability/NamespaceCommentCheck.h"
+#include "readability/UnnecessaryIntermediateVarCheck.h"
+#include "../../../unittests/ASTMatchers/ASTMatchersTest.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -500,6 +502,19 @@
 runCheckOnCode(Input));
 }
 
+TEST(StatementMatcher, HasSuccessor) {
+  using namespace ast_matchers;
+  using namespace matchers;
+
+  StatementMatcher DeclHasSuccessorReturnStmt =
+  declStmt(hasSuccessor(returnStmt()));
+
+  EXPECT_TRUE(
+  matches("void foo() { int bar; return; }", DeclHasSuccessorReturnStmt));
+  EXPECT_TRUE(notMatches("void foo() { int bar; bar++; return; }",
+ DeclHasSuccessorReturnStmt));
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang
Index: test/clang-tidy/readability-unnecessary-intermediate-var.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-unnecessary-intermediate-var.cpp
@@ -0,0 +1,245 @@
+// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t
+
+bool f() {
+  auto test = 1; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (1 == 1);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f2() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test1 == test2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration
+  // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one
+  // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here
+}
+
+bool f3() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (test1 == 2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f4() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test2 == 3);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f5() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (2 == test1);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [read

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:62
 
 - New `bugprone-throw-keyword-missing
   
`_
 check

Please rebase from trunk and use //:doc:// for link.



Comment at: docs/ReleaseNotes.rst:82
+
+  This new check detects unnecessary intermediate variables before `return`
+  statements that return the result of a simple comparison. This check also

Please remove This new check. Please enclose return in ``, also in other places.



Comment at: 
docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst:7
+Detects unnecessary intermediate variables before `return` statements 
returning the
+result of a simple comparison. This checker also suggests to directly inline 
the
+initializer expression of the variable declaration into the `return` 
expression.

Checker -> check.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:28
+  auto File = Context->getCurrentFile();
+  auto Style = format::getStyle(*Context->getOptionsForFile(File).FormatStyle,
+File, "none");

I've just noticed this use of format::getStyle. The problem is that it will use 
the real file system, which is completely wrong for some clang-tidy use cases. 
I guess, we'll either have to roll this back to use a check option or create 
some way for checks to get the formatting style that can be implemented 
correctly by the client, when clang-tidy is used as a library.

I suggest going back to the option and leave a FIXME to investigate the other 
possibility.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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("declStmt1");
+  const Expr *Init1 = Result.Nodes.getNodeAs("init1");
+  const VarDecl *VariableDecl1 = Result.Nodes.getNodeAs("varDecl1");
+  const DeclRefExpr *VarRefLHS1 =
+  Result.Nodes.getNodeAs("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 CFGStatement = BlockItem.getAs();

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon added a comment.

@alexfh Do you think we can merge this? I think I've been through every 
suggestion and it would be nice to finally land the check!


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon updated this revision to Diff 137588.
tbourvon added a comment.

Moves the custom matcher to the check instead of having it in `utils/Matchers.h`


https://reviews.llvm.org/D37014

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
  test/clang-tidy/readability-unnecessary-intermediate-var.cpp
  unittests/clang-tidy/ReadabilityModuleTest.cpp

Index: unittests/clang-tidy/ReadabilityModuleTest.cpp
===
--- unittests/clang-tidy/ReadabilityModuleTest.cpp
+++ unittests/clang-tidy/ReadabilityModuleTest.cpp
@@ -1,6 +1,8 @@
 #include "ClangTidyTest.h"
 #include "readability/BracesAroundStatementsCheck.h"
 #include "readability/NamespaceCommentCheck.h"
+#include "readability/UnnecessaryIntermediateVarCheck.h"
+#include "../../../unittests/ASTMatchers/ASTMatchersTest.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -500,6 +502,19 @@
 runCheckOnCode(Input));
 }
 
+TEST(StatementMatcher, HasSuccessor) {
+  using namespace ast_matchers;
+  using namespace matchers;
+
+  StatementMatcher DeclHasSuccessorReturnStmt =
+  declStmt(hasSuccessor(returnStmt()));
+
+  EXPECT_TRUE(
+  matches("void foo() { int bar; return; }", DeclHasSuccessorReturnStmt));
+  EXPECT_TRUE(notMatches("void foo() { int bar; bar++; return; }",
+ DeclHasSuccessorReturnStmt));
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang
Index: test/clang-tidy/readability-unnecessary-intermediate-var.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-unnecessary-intermediate-var.cpp
@@ -0,0 +1,245 @@
+// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t
+
+bool f() {
+  auto test = 1; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (1 == 1);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f2() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test1 == test2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration
+  // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one
+  // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here
+}
+
+bool f3() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (test1 == 2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f4() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test2 == 3);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f5() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (2 == test1);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variabl

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon updated this revision to Diff 134936.
tbourvon added a comment.

This updates the patch to use `clang::tooling::fixit::getText()` instead of the 
custom `getStmtText`. This also adds a macro unit test.


https://reviews.llvm.org/D37014

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
  test/clang-tidy/readability-unnecessary-intermediate-var.cpp

Index: test/clang-tidy/readability-unnecessary-intermediate-var.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-unnecessary-intermediate-var.cpp
@@ -0,0 +1,245 @@
+// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t
+
+bool f() {
+  auto test = 1; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (1 == 1);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f2() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test1 == test2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration
+  // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one
+  // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here
+}
+
+bool f3() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (test1 == 2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f4() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test2 == 3);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f5() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (2 == test1);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f6() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (3 == test2);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+int foo() { return 1; }
+
+bool f_func() {
+  auto test = foo(); // Test
+  // CHECK-F

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: test/clang-tidy/readability-unnecessary-intermediate-var.cpp:206
+  auto test = 1; // Test
+#ifdef INTERMITTENT_MACRO
+  return (test == 1);

tbourvon wrote:
> lebedev.ri wrote:
> > Tests are nice :)
> > But please, add the test with assert-like macro..
> Sorry, I'm not sure I understand what kind of test you are suggesting... 
> Could you give me a short example?
//Something// like this: (did not test at all, just back of the envelope code)
```
void die();

#ifndef NDEBUG
#define assert(cond) void(0) // i.e. it does nothing
#else
#define assert(cond) (cond) ? void(0) : die();
#endif
```
```
bool some_func();
bool f_preprocessor_macro2() {
  auto test = some_func();
  assert(test); // <- should not be removed regardless of whether NDEBUG is set 
or not
  return test;
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon marked an inline comment as done.
tbourvon added inline comments.



Comment at: test/clang-tidy/readability-unnecessary-intermediate-var.cpp:206
+  auto test = 1; // Test
+#ifdef INTERMITTENT_MACRO
+  return (test == 1);

lebedev.ri wrote:
> Tests are nice :)
> But please, add the test with assert-like macro..
Sorry, I'm not sure I understand what kind of test you are suggesting... Could 
you give me a short example?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: test/clang-tidy/readability-unnecessary-intermediate-var.cpp:206
+  auto test = 1; // Test
+#ifdef INTERMITTENT_MACRO
+  return (test == 1);

Tests are nice :)
But please, add the test with assert-like macro..


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon updated this revision to Diff 131709.
tbourvon added a comment.
Herald added a subscriber: hintonda.

Separated the added matcher and lexer utils into a different diff.
Also added unit tests to make sure we behave as expected when macros get in the 
way of the code we detect.


https://reviews.llvm.org/D37014

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
  test/clang-tidy/readability-unnecessary-intermediate-var.cpp

Index: test/clang-tidy/readability-unnecessary-intermediate-var.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-unnecessary-intermediate-var.cpp
@@ -0,0 +1,230 @@
+// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t
+
+bool f() {
+  auto test = 1; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (1 == 1);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f2() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test1 == test2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration
+  // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one
+  // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here
+}
+
+bool f3() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (test1 == 2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f4() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test2 == 3);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f5() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (2 == test1);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f6() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (3 == test2);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+int foo() {

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thank you for working on this!
Some thoughts below.




Comment at: clang-tidy/readability/CMakeLists.txt:31
   UniqueptrDeleteReleaseCheck.cpp
+  UnnecessaryIntermediateVarCheck.cpp
 

Please upload patches with full context (`-U99`)



Comment at: clang-tidy/utils/LexerUtils.h:26
+/// Get source code text for statement.
+Optional getStmtText(const Stmt* Statement, const SourceManager& 
SM);
+

This should probably be split into new differential, and it should have a 
unit-test.
(And mark that diff as parent of this one)



Comment at: clang-tidy/utils/Matchers.h:54
+// Matches the next statement within the parent statement sequence.
+AST_MATCHER_P(Stmt, hasSuccessor,
+  ast_matchers::internal::Matcher, InnerMatcher) {

This should be split into another new differential, and it should have a 
unit-test.
(And mark that diff as parent of this one)
Also, the docs will need to be updated, which is sadly impossible now, see 
D41455.



Comment at: test/clang-tidy/readability-unnecessary-intermediate-var.cpp:1
+// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t
+

Still no tests for macros, preprocessor definitions.
I do see that it will still do the wrong thing, but i need to see that in the 
tests and release notes.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon marked 4 inline comments as done.
tbourvon added a comment.

(By the way, credits go to @Abpostelnicu for the latest changes regarding 
MaximumLineLength interop with clang-format options)




Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:376
+// expression wouldn't really benefit readability. Therefore we abort.
+if (NewReturnLength > MaximumLineLength) {
+  return;

Abpostelnicu wrote:
> lebedev.ri wrote:
> > Is there really no way to format the fixes, and *then* check the line 
> > length?
> > ```
> > $ clang-tidy --help
> > ...
> >   -format-style=   - 
> >  Style for formatting code around applied 
> > fixes:
> >- 'none' (default) turns off formatting
> >- 'file' (literally 'file', not a 
> > placeholder)
> >  uses .clang-format file in the closest 
> > parent
> >  directory
> >- '{  }' specifies options inline, 
> > e.g.
> >  -format-style='{BasedOnStyle: llvm, 
> > IndentWidth: 8}'
> >- 'llvm', 'google', 'webkit', 'mozilla'
> >  See clang-format documentation for the 
> > up-to-date
> >  information about formatting styles and 
> > options.
> >  This option overrides the 'FormatStyle` 
> > option in
> >  .clang-tidy file, if any.
> > ...
> > ```
> > so `clang-tidy` is at least aware of `clang-format`.
> I think this is doable since I see this in the code:
> 
> https://code.woboq.org/llvm/clang-tools-extra/clang-tidy/ClangTidy.cpp.html#199
> 
> That leads me to think that we can have this before applying the fixes and in 
> case the fix after re-format has a line that violates our rule it gets 
> dropped. I'm gonna update the patch with this new addon.
Regarding this comment, Andi (@Abpostelnicu) and I have analyzed the situation 
and we believe that formatting the replacement before checking the line length 
achieves almost nothing, yet complicates the code a lot.

If we format the replacement before applying the fix, then we're //almost// 
sure that clang-format will split the replacement into multiple lines and that 
it will never go above the maximum line length (except for extremely rare cases 
like 80+ characters identifiers).
Actually, we believe that splitting the return statement into multiple lines 
hinders its readability, and therefore that in cases where the fix would exceed 
the maximum line length, we're better off not applying it, since the main goal 
of this check **is** readability.

clang-format can still be run after the fix is applying, of course.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon updated this revision to Diff 129278.
tbourvon added a comment.

Add retrieval of MaximumLineLength from clang-format options, otherwise 
defaults to 80.
Also add unit tests around the limit case for the line length.


https://reviews.llvm.org/D37014

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  clang-tidy/utils/Matchers.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
  test/clang-tidy/readability-unnecessary-intermediate-var.cpp

Index: test/clang-tidy/readability-unnecessary-intermediate-var.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-unnecessary-intermediate-var.cpp
@@ -0,0 +1,202 @@
+// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t
+
+bool f() {
+  auto test = 1; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (1 == 1);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f2() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test1 == test2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration
+  // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one
+  // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here
+}
+
+bool f3() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (test1 == 2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f4() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test2 == 3);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f5() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (2 == test1);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f6() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (3 == test2);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initializati

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

[Andi via Phabricator via cfe-commits]

Abpostelnicu added inline comments.



Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:376
+// expression wouldn't really benefit readability. Therefore we abort.
+if (NewReturnLength > MaximumLineLength) {
+  return;

lebedev.ri wrote:
> Is there really no way to format the fixes, and *then* check the line length?
> ```
> $ clang-tidy --help
> ...
>   -format-style=   - 
>  Style for formatting code around applied 
> fixes:
>- 'none' (default) turns off formatting
>- 'file' (literally 'file', not a 
> placeholder)
>  uses .clang-format file in the closest 
> parent
>  directory
>- '{  }' specifies options inline, 
> e.g.
>  -format-style='{BasedOnStyle: llvm, 
> IndentWidth: 8}'
>- 'llvm', 'google', 'webkit', 'mozilla'
>  See clang-format documentation for the 
> up-to-date
>  information about formatting styles and 
> options.
>  This option overrides the 'FormatStyle` 
> option in
>  .clang-tidy file, if any.
> ...
> ```
> so `clang-tidy` is at least aware of `clang-format`.
I think this is doable since I see this in the code:

https://code.woboq.org/llvm/clang-tools-extra/clang-tidy/ClangTidy.cpp.html#199

That leads me to think that we can have this before applying the fixes and in 
case the fix after re-format has a line that violates our rule it gets dropped. 
I'm gonna update the patch with this new addon.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:376
+// expression wouldn't really benefit readability. Therefore we abort.
+if (NewReturnLength > MaximumLineLength) {
+  return;

Is there really no way to format the fixes, and *then* check the line length?
```
$ clang-tidy --help
...
  -format-style=   - 
 Style for formatting code around applied fixes:
   - 'none' (default) turns off formatting
   - 'file' (literally 'file', not a 
placeholder)
 uses .clang-format file in the closest 
parent
 directory
   - '{  }' specifies options inline, e.g.
 -format-style='{BasedOnStyle: llvm, 
IndentWidth: 8}'
   - 'llvm', 'google', 'webkit', 'mozilla'
 See clang-format documentation for the 
up-to-date
 information about formatting styles and 
options.
 This option overrides the 'FormatStyle` option 
in
 .clang-tidy file, if any.
...
```
so `clang-tidy` is at least aware of `clang-format`.



Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.h:30
+  : ClangTidyCheck(Name, Context),
+MaximumLineLength(Options.get("MaximumLineLength", 100)) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;

It would be better to default to `80`, especially if it can't be read from 
`.clang-tidy`.



Comment at: test/clang-tidy/readability-unnecessary-intermediate-var.cpp:172
+bool f_long_expression() {
+  auto test = "this is a very very very very very very very very very long 
expression to test max line length detection";
+  return (test == "");

Please add edge-cases, i.e. just below `MaximumLineLength`, exactly 
`MaximumLineLength`, ...


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon updated this revision to Diff 112356.
tbourvon marked an inline comment as done.
tbourvon added a comment.

Forgot to move the comments on small matchers as suggested by review.


https://reviews.llvm.org/D37014

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  clang-tidy/utils/Matchers.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
  test/clang-tidy/readability-unnecessary-intermediate-var.cpp

Index: test/clang-tidy/readability-unnecessary-intermediate-var.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-unnecessary-intermediate-var.cpp
@@ -0,0 +1,174 @@
+// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t
+
+bool f() {
+  auto test = 1; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (1 == 1);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f2() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test1 == test2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration
+  // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one
+  // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here
+}
+
+bool f3() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (test1 == 2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f4() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test2 == 3);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f5() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (2 == test1);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f6() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (3 == test2);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+int foo() { re

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon marked 14 inline comments as done.
tbourvon added inline comments.



Comment at: clang-tidy/readability/UselessIntermediateVarCheck.h:29
+  : ClangTidyCheck(Name, Context),
+MaximumLineLength(Options.get("MaximumLineLength", 100)) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;

JonasToth wrote:
> It would be nice, if there is a way to get this information from a 
> clang-format file. 
There is no easy way to get this information unless you want to use all the 
LibFormat stuff that includes finding the .clang-format file, etc... It doesn't 
seem like it is the responsibility of this checker to take care of that.



Comment at: test/clang-tidy/readability-useless-intermediate-var.cpp:1
+// RUN: %check_clang_tidy %s readability-useless-intermediate-var %t
+

JonasToth wrote:
> 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
Your 4th point is not something possible with this check. For now it only looks 
at `return` statements with comparisons.
As for the 5th, it doesn't really matter what the expression we are looking at 
is, so this case is already covered by the very long string literal in the last 
test.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon updated this revision to Diff 112351.
tbourvon added a comment.

Fixing the reviewers' remarks, mainly formatting and const-correctness, as well 
as adding a few more tests.


https://reviews.llvm.org/D37014

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
  clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  clang-tidy/utils/Matchers.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
  test/clang-tidy/readability-unnecessary-intermediate-var.cpp

Index: test/clang-tidy/readability-unnecessary-intermediate-var.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-unnecessary-intermediate-var.cpp
@@ -0,0 +1,174 @@
+// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t
+
+bool f() {
+  auto test = 1; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (1 == 1);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f2() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test1 == test2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration
+  // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one
+  // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here
+}
+
+bool f3() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (test1 == 2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f4() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test2 == 3);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f5() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (2 == test1);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f6() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (3 == test2);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+int foo() { return

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon added a comment.

In https://reviews.llvm.org/D37014#850088, @lebedev.ri wrote:

> In https://reviews.llvm.org/D37014#850064, @tbourvon wrote:
>
> > In https://reviews.llvm.org/D37014#849157, @lebedev.ri wrote:
> >
> > > Please add the following test: (and make sure that it does the right 
> > > thing :))
> > >
> > >   bool f_with_preproc_condition() {
> > > auto test = 42;
> > > assert(test == 42);
> > > return test;
> > >   }
> > >
> > >
> > > I.e. if `-DNDEBUG` is present, variable is not needed, but if `-DNDEBUG` 
> > > is *NOT* present...
> >
> >
> > Shouldn't we ignore these cases whether or not `-DNDEBUG` is present?
>
>
> That's *exactly* what i was talking about.
>
> > If we apply the fix here and remove the variable while we have `-DNDEBUG`, 
> > and then remove this define, then we'll get a compiler error for `test` not 
> > found.
> >  In this case it can be fixed fairly easily, but this could in fact 
> > introduce bugs with more complex conditional macros and preprocessor 
> > directives. For example:
> > 
> >   #ifdef WIN32
> >   #define MY_MACRO(test) test = 0
> >   #else
> >   #define MY_MACRO(test) /* nothing */
> >   #endif
> >   
> >   bool f() {
> > auto test = 42;
> > MY_MACRO(test);
> > return (test == 42);
> >   }
> > 
> > 
> > If we want to ignore these cases not matter what, which seems to me like 
> > the most logical and reasonable thing to do, we need to be able to know if 
> > there are preprocessor directives between the variable declaration and the 
> > `return` statement.
> >  In order to do that, we would need to be able to get the 
> > `PreprocessingRecord` of the current compilation in order to call 
> > getPreprocessedEntitiesInRange() 
> > ,
> >  but AFAICT this cannot be accessed from clang-tidy checks at the moment. 
> > Should we introduce a way to reach that object from checks?
>
> Currently, there is a `registerPPCallbacks()` function in `ClangTidyCheck` 
> class,  which allows you to register a subclass of `PPCallbacks`, and 
> manually handle all the preprocessor thingies.
>  That being said, getPreprocessedEntitiesInRange() 
> 
>  looks interesting, and it *might* help me in https://reviews.llvm.org/D36836.


IMO it makes more sense to expose `PreprocessingRecord`, as registering 
callbacks and maintaining a private inner database for every check not only 
isn't ideal in terms of performance, but also duplicates logic and could lead 
to inconsistencies.
I think I'm going to work on getting this object accessible through 
`MatchResult` and `MatchFinder` so that it is accessible from both checks and 
matchers. Probably in a separate patch. Please let me know if you have any more 
thoughts on this.


Repository:
  rL LLVM

https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D37014#850064, @tbourvon wrote:

> In https://reviews.llvm.org/D37014#849157, @lebedev.ri wrote:
>
> > Please add the following test: (and make sure that it does the right thing 
> > :))
> >
> >   bool f_with_preproc_condition() {
> > auto test = 42;
> > assert(test == 42);
> > return test;
> >   }
> >
> >
> > I.e. if `-DNDEBUG` is present, variable is not needed, but if `-DNDEBUG` is 
> > *NOT* present...
>
>
> Shouldn't we ignore these cases whether or not `-DNDEBUG` is present?


That's *exactly* what i was talking about.

> If we apply the fix here and remove the variable while we have `-DNDEBUG`, 
> and then remove this define, then we'll get a compiler error for `test` not 
> found.
>  In this case it can be fixed fairly easily, but this could in fact introduce 
> bugs with more complex conditional macros and preprocessor directives. For 
> example:
> 
>   #ifdef WIN32
>   #define MY_MACRO(test) test = 0
>   #else
>   #define MY_MACRO(test) /* nothing */
>   #endif
>   
>   bool f() {
> auto test = 42;
> MY_MACRO(test);
> return (test == 42);
>   }
> 
> 
> If we want to ignore these cases not matter what, which seems to me like the 
> most logical and reasonable thing to do, we need to be able to know if there 
> are preprocessor directives between the variable declaration and the `return` 
> statement.
>  In order to do that, we would need to be able to get the 
> `PreprocessingRecord` of the current compilation in order to call 
> getPreprocessedEntitiesInRange() 
> ,
>  but AFAICT this cannot be accessed from clang-tidy checks at the moment. 
> Should we introduce a way to reach that object from checks?

Currently, there is a `registerPPCallbacks()` function in `ClangTidyCheck` 
class,  which allows you to register a subclass of `PPCallbacks`, and manually 
handle all the preprocessor thingies.
That being said, getPreprocessedEntitiesInRange() 

 looks interesting, and it *might* help me in https://reviews.llvm.org/D36836.


Repository:
  rL LLVM

https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon added a comment.

In https://reviews.llvm.org/D37014#849157, @lebedev.ri wrote:

> Please add the following test: (and make sure that it does the right thing :))
>
>   bool f_with_preproc_condition() {
> auto test = 42;
> assert(test == 42);
> return test;
>   }
>
>
> I.e. if `-DNDEBUG` is present, variable is not needed, but if `-DNDEBUG` is 
> *NOT* present...


Shouldn't we ignore these cases whether or not `-DNDEBUG` is present? If we 
apply the fix here and remove the variable while we have `-DNDEBUG`, and then 
remove this define, then we'll get a compiler error for `test` not found.
In this case it can be fixed fairly easily, but this could in fact introduce 
bugs with more complex conditional macros and preprocessor directives. For 
example:

  #ifdef WIN32
  #define MY_MACRO(test) test = 0
  #else
  #define MY_MACRO(test) /* nothing */
  
  bool f() {
auto test = 42;
MY_MACRO(test);
return (test == 42)
  }

If we want to ignore these cases not matter what, which seems to me like the 
most logical and reasonable thing to do, we need to be able to know if there 
are preprocessor directives between the variable declaration and the `return` 
statement.
In order to do that, we would need to be able to get the `PreprocessingRecord` 
of the current compilation in order to call getPreprocessedEntitiesInRange() 
,
 but AFAICT this cannot be accessed from clang-tidy checks at the moment. 
Should we introduce a way to reach that object from checks?


Repository:
  rL LLVM

https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Please add the following test: (and make sure that it does the right thing :))

  bool f_with_preproc_condition() {
auto test = 42;
assert(test == 42);
return test;
  }

I.e. if `-DNDEBUG` is present, variable is not needed, but if `-DNDEBUG` is 
*NOT* present...


Repository:
  rL LLVM

https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:60
 
+- New `readability-useless-intermediate-var
+  
`_
 check

Please place new checks in alphabetical order.



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

JonasToth wrote:
> maybe "useless" should be replaced with "unnecessary". but thats only my 
> opinion.
Checker -> check.
Please enclose return (in statement context) in ``.
Same for other places.


Repository:
  rL LLVM

https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 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 StatementCFG
+  = CFG::buildCFG(nullptr, const_cast(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 m

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon created this revision.
Herald added subscribers: xazax.hun, JDevlieghere, mgorny.

This patch adds a checker to detect patterns of the following form:

  auto IntermediateVar = foo();
  return (IntermediateVar == 1);

and suggests to turn them into:

  return (foo() == 1);

The reasoning behind this checker is that this kind of pattern is useless and 
lowers readability as long as the return statement remains rather short.
The idea of this checker was suggested to me by Sylvestre Ledru.


https://reviews.llvm.org/D37014

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/UselessIntermediateVarCheck.cpp
  clang-tidy/readability/UselessIntermediateVarCheck.h
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  clang-tidy/utils/Matchers.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-useless-intermediate-var.rst
  test/clang-tidy/readability-useless-intermediate-var.cpp

Index: test/clang-tidy/readability-useless-intermediate-var.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-useless-intermediate-var.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s readability-useless-intermediate-var %t
+
+bool f() {
+  auto test = 1; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (1 == 1);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: intermediate variable 'test' is useless [readability-useless-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f2() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test1 == test2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: intermediate variable 'test1' is useless [readability-useless-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-useless-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration
+  // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one
+  // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here
+}
+
+bool f3() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (test1 == 2);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: intermediate variable 'test1' is useless [readability-useless-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f4() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (test2 == 3);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: intermediate variable 'test2' is useless [readability-useless-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f5() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  auto test2 = 2; // Test2
+  return (2 == test1);
+  // CHECK-FIXES: {{^}}  return (1 == 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: intermediate variable 'test1' is useless [readability-useless-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison
+  // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration
+  // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here
+}
+
+bool f6() {
+  auto test1 = 1; // Test1
+  auto test2 = 2; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (3 == test2);
+  // CHECK-FIXES: {{^}}  return (2 == 3);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: intermediate variable 'test2' is useless [readability-useless-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when ret

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

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon added a comment.

Some more precisions I kept out of the revision body for clarity:

The checker contains a configuration option to set the maximum line length for 
the fixed return statement, so that we make sure this checker always 
contributes to improving readability and not the contrary.
The top-most non-trivial expression of the return statement has to be a 
comparison operator.

This checker tries to catch as many corner cases as possible. Most if not all 
of them are showcased in the tests, but the main ones are:

- It handles double variable declarations like this:

  auto Var1 = 1;
  auto Var2 = 2;
  return (Var1 < Var2);

so that the checker doesn't have to be run twice to fix these cases.

- It always preserves order of execution:

  auto Var = step1();
  return (step2() > Var);

will correctly be transformed into:

  return (step1() < step2()); // notice the comparison operator inversion



- It never duplicates code execution:

  auto Var = foo();
  return (Var == Var);

will not be matched.

I did not feel it was a good idea to include this in the user-facing checker 
documentation because it seems to me that the user of the checker does not need 
to worry about these exceptions as long as they are covered. The user should 
only be interested in the idea behind the checker and how it can help.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits