[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon added inline comments.



Comment at: clang-tidy/utils/Matchers.h:16
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/CFG.h"
 

aaron.ballman wrote:
> This will require linking in the clangAnalysis library as well; are we sure 
> we want to take on this dependency here for all matchers?
Do you have a specific solution in mind? We could make the matcher local to the 
check it is being used in (see D37014), but I think it could prove useful for 
other checks...



Comment at: clang-tidy/utils/Matchers.h:58-60
+  // We get the first parent, making sure that we're not in a case statement
+  // not in a compound statement directly inside a switch, because this causes
+  // the buildCFG call to crash.

aaron.ballman wrote:
> Why does it cause the crash? Should that crash not be fixed instead of 
> applying this workaround?
I'm not entirely sure if this is expected behavior or not. In terms of AST, 
`switch` statements are a bit special in terms of how they are represented 
(each case contains all the subsequent cases as its children IIRC).
There probably is a way to make the CFG work in these cases, but I honestly 
don't have the time to look into that and attempt a fix. Couldn't this be good 
enough for now, maybe with a FIXME?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42624



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

[PATCH] D42623: [clang-tidy] Add a Lexer util to get the source text of a statement

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon abandoned this revision.
tbourvon added inline comments.



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

alexfh wrote:
> aaron.ballman wrote:
> > Formatting (elsewhere as well).
> > 
> > You should run the patch under clang-format to handle this sort of thing.
> Have you seen clang::tooling::fixit::getText? It should cover this use case.
Hadn't seen that! Seems to be making this whole patch obsolete, I'm closing it


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42623



___
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 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

[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

[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] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon added a comment.

@alexfh What is your opinion regarding adding this dependency?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42624



___
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 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 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

[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.

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

[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

[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 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 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 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

[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 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

[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] D42623: [clang-tidy] Add a Lexer util to get the source text of a statement

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon created this revision.
tbourvon added a reviewer: lebedev.ri.
tbourvon added a project: clang-tools-extra.
Herald added subscribers: hintonda, xazax.hun, mgorny.

This is a simple Lexer util to get the source text of a statement given as 
parameter. This is needed for https://reviews.llvm.org/D37014.


https://reviews.llvm.org/D42623

Files:
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  unittests/clang-tidy/CMakeLists.txt
  unittests/clang-tidy/LexerUtilsTest.cpp

Index: unittests/clang-tidy/LexerUtilsTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/LexerUtilsTest.cpp
@@ -0,0 +1,51 @@
+#include "ClangTidyTest.h"
+#include "clang/Tooling/Tooling.h"
+#include "utils/LexerUtils.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+class GetStmtTextTest : public ::testing::Test {
+protected:
+  GetStmtTextTest()
+: Unit(CheckASTUnitNotNull(tooling::buildASTFromCode("void foo() { int bar = 0; }"))),
+  SM(Unit->getSourceManager())
+  {}
+
+  std::unique_ptr CheckASTUnitNotNull(std::unique_ptr Unit) {
+EXPECT_TRUE(Unit);
+return Unit;
+  }
+
+  std::unique_ptr Unit;
+  SourceManager& SM;
+};
+
+TEST_F(GetStmtTextTest, NullStatement) {
+  EXPECT_EQ(llvm::NoneType(), utils::lexer::getStmtText(nullptr, SM));
+}
+
+TEST_F(GetStmtTextTest, SimpleStatement) {
+  auto FooIdent = &Unit->getASTContext().Idents.getOwn("foo");
+  auto FooDeclName = Unit->getASTContext().DeclarationNames.getIdentifier(FooIdent);
+  auto FooLookup = Unit->getASTContext().getTranslationUnitDecl()->lookup(FooDeclName);
+  auto FooFunction = dyn_cast_or_null(FooLookup.front());
+  EXPECT_TRUE(FooFunction);
+
+  auto FooBody = dyn_cast_or_null(FooFunction->getBody());
+  EXPECT_TRUE(FooBody);
+  EXPECT_TRUE(FooBody->body_begin());
+
+  auto DeclStmt = *FooBody->body_begin();
+
+
+  auto Res = utils::lexer::getStmtText(DeclStmt, SM);
+  EXPECT_TRUE(Res.hasValue());
+  EXPECT_EQ("int bar = 0;", *Res);
+}
+
+} // namespace test
+} // namespace tidy
+} // namespace clang
Index: unittests/clang-tidy/CMakeLists.txt
===
--- unittests/clang-tidy/CMakeLists.txt
+++ unittests/clang-tidy/CMakeLists.txt
@@ -11,6 +11,7 @@
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
   GoogleModuleTest.cpp
+  LexerUtilsTest.cpp
   LLVMModuleTest.cpp
   NamespaceAliaserTest.cpp
   ObjCModuleTest.cpp
Index: clang-tidy/utils/LexerUtils.h
===
--- clang-tidy/utils/LexerUtils.h
+++ clang-tidy/utils/LexerUtils.h
@@ -22,6 +22,9 @@
 Token getPreviousToken(const ASTContext &Context, SourceLocation Location,
bool SkipComments = true);
 
+/// Get source code text for statement.
+Optional getStmtText(const Stmt* Statement, const SourceManager& SM);
+
 } // namespace lexer
 } // namespace utils
 } // namespace tidy
Index: clang-tidy/utils/LexerUtils.cpp
===
--- clang-tidy/utils/LexerUtils.cpp
+++ clang-tidy/utils/LexerUtils.cpp
@@ -35,6 +35,20 @@
   return Token;
 }
 
+Optional getStmtText(const Stmt* Statement, const SourceManager& SM) {
+  if (!Statement) {
+return llvm::NoneType();
+  }
+
+  bool Error = false;
+  auto Ret = Lexer::getSourceText(
+CharSourceRange::getTokenRange(Statement->getSourceRange()),
+SM, LangOptions(),
+&Error);
+
+  return Error ? llvm::NoneType() : Optional(Ret);
+}
+
 } // namespace lexer
 } // namespace utils
 } // namespace tidy
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon created this revision.
tbourvon added a reviewer: lebedev.ri.
tbourvon added a project: clang-tools-extra.
Herald added subscribers: hintonda, xazax.hun, mgorny.

This adds a utility matcher (which is placed in `util/Matchers.h`, because it 
uses functions from `ASTMatchFinder.h` which cannot be used from 
`ASTMatchers.h`) used by https://reviews.llvm.org/D37014).


https://reviews.llvm.org/D42624

Files:
  clang-tidy/utils/Matchers.h
  unittests/clang-tidy/CMakeLists.txt
  unittests/clang-tidy/MatchersUtilsTest.cpp

Index: unittests/clang-tidy/MatchersUtilsTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/MatchersUtilsTest.cpp
@@ -0,0 +1,25 @@
+#include "../../../unittests/ASTMatchers/ASTMatchersTest.h"
+#include "utils/Matchers.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace ast_matchers;
+using namespace matchers;
+
+TEST(StatementMatcher, HasSuccessor) {
+  StatementMatcher DeclHasSuccessorReturnStmt =
+declStmt(hasSuccessor(returnStmt()));
+
+  EXPECT_TRUE(matches(
+"void foo() { int bar; return; }",
+DeclHasSuccessorReturnStmt));
+  EXPECT_TRUE(notMatches(
+"void foo() { int bar; bar++; return; }",
+DeclHasSuccessorReturnStmt));
+}
+
+}
+}
+}
Index: unittests/clang-tidy/CMakeLists.txt
===
--- unittests/clang-tidy/CMakeLists.txt
+++ unittests/clang-tidy/CMakeLists.txt
@@ -12,6 +12,7 @@
   IncludeInserterTest.cpp
   GoogleModuleTest.cpp
   LLVMModuleTest.cpp
+  MatchersUtilsTest.cpp
   NamespaceAliaserTest.cpp
   ObjCModuleTest.cpp
   OverlappingReplacementsTest.cpp
Index: clang-tidy/utils/Matchers.h
===
--- clang-tidy/utils/Matchers.h
+++ clang-tidy/utils/Matchers.h
@@ -12,6 +12,8 @@
 
 #include "TypeTraits.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/CFG.h"
 
 namespace clang {
 namespace tidy {
@@ -48,6 +50,67 @@
   return referenceType(pointee(qualType(isConstQualified(;
 }
 
+// Matches the next statement within the parent statement sequence.
+AST_MATCHER_P(Stmt, hasSuccessor,
+  ast_matchers::internal::Matcher, InnerMatcher) {
+  using namespace ast_matchers;
+
+  // We get the first parent, making sure that we're not in a case statement
+  // not in a compound statement directly inside a switch, because this causes
+  // the buildCFG call to crash.
+  auto Parent = selectFirst(
+"parent",
+match(
+  stmt(hasAncestor(stmt(
+unless(caseStmt()),
+unless(compoundStmt(hasParent(switchStmt(,
+stmt().bind("parent",
+Node, Finder->getASTContext()));
+
+  // We build a Control Flow Graph (CFG) from the parent statement.
+  std::unique_ptr StatementCFG =
+CFG::buildCFG(nullptr, const_cast(Parent), &Finder->getASTContext(),
+  CFG::BuildOptions());
+
+  if (!StatementCFG) {
+return false;
+  }
+
+  // We iterate through all the CFGBlocks, which basically means that we go over
+  // all the possible branches of the code and therefore cover all statements.
+  for (auto& Block : *StatementCFG) {
+if (!Block) {
+  continue;
+}
+
+// We iterate through all the statements of the block.
+bool ReturnNextStmt = false;
+for (auto& BlockItem : *Block) {
+  Optional CFGStatement = BlockItem.getAs();
+  if (!CFGStatement) {
+if (ReturnNextStmt) {
+  return false;
+}
+
+continue;
+  }
+
+  // If we found the next statement, we apply the inner matcher and return
+  // the result.
+  if (ReturnNextStmt) {
+return InnerMatcher.matches(*CFGStatement->getStmt(), Finder, Builder);
+  }
+
+  if (CFGStatement->getStmt() == &Node) {
+ReturnNextStmt = true;
+  }
+}
+  }
+
+  // If we didn't find a successor, we just return false.
+  return false;
+}
+
 } // namespace matchers
 } // namespace tidy
 } // namespace clang
___
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