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

[tbourvon]

Summary

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.

[tbourvon]

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.

[JonasToth]

Hi :)

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

clang-tidy/readability/UselessIntermediateVarCheck.h

29 ↗

(On Diff #112179)

It would be nice, if there is a way to get this information from a clang-format file.

[JonasToth]

clang-tidy/readability/UselessIntermediateVarCheck.cpp

24 ↗

(On Diff #112179)

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.

62 ↗

(On Diff #112179)

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.

186 ↗

(On Diff #112179)

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

236 ↗

(On Diff #112179)

const auto ?

290 ↗

(On Diff #112179)

some const is possible for this an the following variables.

335 ↗

(On Diff #112179)

const auto?

350 ↗

(On Diff #112179)

const

clang-tidy/readability/UselessIntermediateVarCheck.h

47 ↗

(On Diff #112179)

Maybe a SmallSet? the optimized datastructures from llvm would save dynamic memory allocations.

clang-tidy/utils/Matchers.h

67 ↗

(On Diff #112179)

formatted? usually the = ends up on the same line

docs/ReleaseNotes.rst

63

maybe "useless" should be replaced with "unnecessary". but thats only my opinion.

docs/clang-tidy/checks/readability-useless-intermediate-var.rst

14 ↗

(On Diff #112179)

especially to avoid magic numbers, this kind of code could be more readable.

test/clang-tidy/readability-useless-intermediate-var.cpp

1 ↗

(On Diff #112179)

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

[Eugene.Zelenko]

docs/ReleaseNotes.rst

60

Please place new checks in alphabetical order.

63

Checker -> check.
Please enclose return (in statement context) in ``.
Same for other places.

[lebedev.ri]

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

[tbourvon]

In 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 */
#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?

[lebedev.ri]

In D37014#850064, @tbourvon wrote:

In 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 D36836.

[tbourvon]

In D37014#850088, @lebedev.ri wrote:

In D37014#850064, @tbourvon wrote:

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

[tbourvon]

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

[tbourvon]

clang-tidy/readability/UselessIntermediateVarCheck.h

29 ↗

(On Diff #112179)

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.

test/clang-tidy/readability-useless-intermediate-var.cpp

1 ↗

(On Diff #112179)

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.

[tbourvon]

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

[lebedev.ri]

clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp

377

Is there really no way to format the fixes, and *then* check the line length?

$ clang-tidy --help
...
  -format-style=<string>       - 
                                 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
                                   - '{ <json> }' 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.

clang-tidy/readability/UnnecessaryIntermediateVarCheck.h

31

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

test/clang-tidy/readability-unnecessary-intermediate-var.cpp

173

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

[Abpostelnicu]

clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp

377

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.

[tbourvon]

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

[tbourvon]

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

clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp

377

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.

[lebedev.ri]

Thank you for working on this!
Some thoughts below.

clang-tidy/readability/CMakeLists.txt

31

Please upload patches with full context (-U999999)

clang-tidy/utils/LexerUtils.h

26 ↗

(On Diff #129278)

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

clang-tidy/utils/Matchers.h

54 ↗

(On Diff #129278)

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.

test/clang-tidy/readability-unnecessary-intermediate-var.cpp

2

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.

[tbourvon]

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.

[lebedev.ri]

test/clang-tidy/readability-unnecessary-intermediate-var.cpp

207

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

[tbourvon]

test/clang-tidy/readability-unnecessary-intermediate-var.cpp

207

Sorry, I'm not sure I understand what kind of test you are suggesting... Could you give me a short example?

[lebedev.ri]

test/clang-tidy/readability-unnecessary-intermediate-var.cpp

207

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;
}

[tbourvon]

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

[tbourvon]

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

[tbourvon]

@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!

[aaron.ballman]

clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp

28–31

Please do not use auto here as the type is not explicitly spelled out in the initializer. Same comment applies elsewhere.

33

Elide braces.

190

This should be using a %select in the diagnostic.

235–242

Can use const auto * because the type is explicitly spelled out in the initialization. Same comment applies elsewhere.

275

Elide braces (applies elsewhere).

clang-tidy/readability/UnnecessaryIntermediateVarCheck.h

52

Can this be const auto &?

58

Same here.

docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst

5

Underline is of incorrect length.

[alexfh]

clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp

29

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.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

64–65

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

148

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

docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst

8

Checker -> check.

[tbourvon]

Fixed review comments

[tbourvon]

docs/ReleaseNotes.rst

64–65

I'm sorry, what do you mean by "use :doc: for link"?

[Eugene.Zelenko]

docs/ReleaseNotes.rst

145

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

[tbourvon]

Order and link fixes in the release notes

[Eugene.Zelenko]

clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp

1

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

clang-tidy/readability/UnnecessaryIntermediateVarCheck.h

1

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

docs/ReleaseNotes.rst

128

Final underscore is not needed.

docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst

6

Please add missing `. Same below.

[alexfh]

clang-tidy/readability/UnnecessaryIntermediateVarCheck.h

27

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

clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   RedundantMemberInitCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
   StaticAccessedThroughInstanceCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   StringCompareCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
+  UnnecessaryIntermediateVarCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "StaticAccessedThroughInstanceCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "StringCompareCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
+#include "UnnecessaryIntermediateVarCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<RedundantStringCStrCheck>(
         "readability-redundant-string-cstr");
     CheckFactories.registerCheck<RedundantStringInitCheck>(
         "readability-redundant-string-init");
     CheckFactories.registerCheck<SimplifyBooleanExprCheck>(
         "readability-simplify-boolean-expr");
     CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(
         "readability-uniqueptr-delete-release");
+    CheckFactories.registerCheck<UnnecessaryIntermediateVarCheck>(
+        "readability-unnecessary-intermediate-var");
   }
 };
 
 // Register the ReadabilityModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ReadabilityModule>
     X("readability-module", "Adds readability-related checks.");
 
 } // namespace readability
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ReadabilityModule.
 volatile int ReadabilityModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/readability/UnnecessaryIntermediateVarCheck.h

+//===--- UnnecessaryIntermediateVarCheck.h - clang-tidy--------------*- C++
+//-*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNNECESSARY_INTERMEDIATE_VAR_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNNECESSARY_INTERMEDIATE_VAR_H
+
+#include "../ClangTidy.h"
+#include "clang/Format/Format.h"
+#include "llvm/ADT/SmallSet.h"
+#include "llvm/Support/Process.h"
+#include "llvm/Support/Signals.h"
+#include "clang/Analysis/CFG.h"
+
+namespace clang {
+namespace tidy {
+
+namespace matchers {
+
+// Matches the next statement within the parent statement sequence.
+AST_MATCHER_P(Stmt, hasSuccessor, ast_matchers::internal::Matcher<Stmt>,
+              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<Stmt>(
+      "parent",
+      match(stmt(hasAncestor(stmt(unless(caseStmt()),
+                                  unless(compoundStmt(hasParent(switchStmt()))),
+                                  stmt().bind("parent")))),
+            Node, Finder->getASTContext()));
+
+  // We build a CFG from the parent statement.
+  std::unique_ptr<CFG> StatementCFG =
+      CFG::buildCFG(nullptr, const_cast<Stmt *>(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 (const auto &Block : *StatementCFG) {
+    if (!Block)
+      continue;
+
+    // We iterate through all the statements of the block.
+    bool ReturnNextStmt = false;
+    for (const auto &BlockItem : *Block) {
+      Optional<CFGStmt> CFGStatement = BlockItem.getAs<CFGStmt>();
+      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 readability {
+
+/// This checker detects unnecessary intermediate variables used to store the
+/// result of an expression just before using it in a return statement.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-unnecessary-intermediate-var.html
+class UnnecessaryIntermediateVarCheck : public ClangTidyCheck {
+public:
+  UnnecessaryIntermediateVarCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  bool checkReplacement(const ClangTidyContext *Context, StringRef Replacement);
+  void emitMainWarning(const VarDecl *VarDecl1,
+                       const VarDecl *VarDecl2 = nullptr);
+  void emitUsageInComparisonNote(const DeclRefExpr *VarRef,
+                                 const bool IsPlural);
+  void emitVarDeclRemovalNote(const VarDecl *VarDecl1,
+                              const DeclStmt *DeclStmt1,
+                              const VarDecl *VarDecl2 = nullptr,
+                              const DeclStmt *DeclStmt2 = nullptr);
+  void emitReturnReplacementNote(const Expr *LHS,
+                                 const StringRef LHSReplacement,
+                                 const Expr *RHS = nullptr,
+                                 const StringRef RHSReplacement = StringRef(),
+                                 const BinaryOperator *ReverseBinOp = nullptr);
+
+  unsigned MaximumLineLength;
+  llvm::SmallSet<const DeclStmt *, 10> CheckedDeclStmt;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNNECESSARY_INTERMEDIATE_VAR_H

clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp

+//===--- UnnecessaryIntermediateVarCheck.cpp -
+// clang-tidy----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UnnecessaryIntermediateVarCheck.h"
+#include "../utils/Matchers.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+UnnecessaryIntermediateVarCheck::UnnecessaryIntermediateVarCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {
+  // FIXME: try to get the max length value from the clang-format configuration
+  MaximumLineLength = Options.get("MaximumLineLength", 80);
+}
+  
+void UnnecessaryIntermediateVarCheck::registerMatchers(MatchFinder *Finder) {
+  // We match a direct declaration reference expression pointing
+  // to the variable declaration 1 as LHS.
+  const auto DirectDeclRefExprLHS1 =
+      ignoringImplicit(ignoringParenCasts(declRefExpr(
+          to(equalsBoundNode("varDecl1")), expr().bind("declRefExprLHS1"))));
+
+  // We match a direct declaration reference expression pointing
+  // to the variable declaration 1 as RHS.
+  const auto DirectDeclRefExprRHS1 =
+      ignoringImplicit(ignoringParenCasts(declRefExpr(
+          to(equalsBoundNode("varDecl1")), expr().bind("declRefExprRHS1"))));
+
+  // We match a declaration reference expression in any descendant
+  // pointing to variable declaration 1.
+  const auto NoIndirectDeclRefExpr1 =
+      unless(hasDescendant(declRefExpr(to(equalsBoundNode("varDecl1")))));
+
+  // We match a direct declaration reference expression pointing
+  // to the variable declaration 2 as LHS.
+  const auto DirectDeclRefExprLHS2 =
+      ignoringImplicit(ignoringParenCasts(declRefExpr(
+          to(equalsBoundNode("varDecl2")), expr().bind("declRefExprLHS2"))));
+
+  // We match a direct declaration reference expression pointing
+  // to the variable declaration 2 as RHS.
+  const auto DirectDeclRefExprRHS2 =
+      ignoringImplicit(ignoringParenCasts(declRefExpr(
+          to(equalsBoundNode("varDecl2")), expr().bind("declRefExprRHS2"))));
+
+  // We match a declaration reference expression in any descendant
+  // pointing to variable declaration 2.
+  const auto NoIndirectDeclRefExpr2 =
+      unless(hasDescendant(declRefExpr(to(equalsBoundNode("varDecl2")))));
+
+  const auto HasVarDecl1 =
+      // We match a single declaration which is a variable declaration,
+      hasSingleDecl(varDecl(
+          // which has an initializer,
+          hasInitializer(allOf(NoIndirectDeclRefExpr2, expr().bind("init1"))),
+          // and which isn't static.
+          unless(isStaticStorageClass()), decl().bind("varDecl1")));
+
+  const auto HasVarDecl2 =
+      // We match a single declaration which is a variable declaration,
+      hasSingleDecl(varDecl(
+          // which has an initializer,
+          hasInitializer(allOf(NoIndirectDeclRefExpr1, expr().bind("init2"))),
+          // and which isn't static.
+          unless(isStaticStorageClass()), decl().bind("varDecl2")));
+
+  const auto ReturnStmt1 =
+      // We match a return statement,
+      returnStmt(
+          stmt().bind("returnStmt1"),
+
+          // which has a return value which is a binary operator,
+          hasReturnValue(ignoringImplicit(ignoringParenCasts(
+              binaryOperator(expr().bind("binOp"),
+
+                             // which is a comparison operator,
+                             matchers::isComparisonOperator(),
+
+                             // which may contain a direct reference to var decl
+                             // 1 on only one side.
+                             anyOf(allOf(hasLHS(DirectDeclRefExprLHS1),
+                                         hasRHS(NoIndirectDeclRefExpr1)),
+                                   allOf(hasLHS(NoIndirectDeclRefExpr1),
+                                         hasRHS(DirectDeclRefExprRHS1))))))));
+
+  const auto ReturnStmt2 =
+      // We match a return statement,
+      returnStmt(
+          stmt().bind("returnStmt2"),
+
+          // which has a return value which is a binary operator,
+          hasReturnValue(ignoringImplicit(ignoringParenCasts(binaryOperator(
+              expr().bind("binOp"),
+
+              // which is a comparison operator,
+              matchers::isComparisonOperator(),
+
+              // which may contain a direct reference to a var decl on one side,
+              // as long as there is no indirect reference to the same var decl
+              // on the other size.
+              anyOf(
+                  allOf(
+                      hasLHS(DirectDeclRefExprLHS1),
+                      hasRHS(allOf(NoIndirectDeclRefExpr1,
+                                   anyOf(DirectDeclRefExprRHS2, anything())))),
+
+                  allOf(
+                      hasLHS(DirectDeclRefExprLHS2),
+                      hasRHS(allOf(NoIndirectDeclRefExpr2,
+                                   anyOf(DirectDeclRefExprRHS1, anything())))),
+
+                  allOf(hasLHS(allOf(NoIndirectDeclRefExpr1,
+                                     anyOf(DirectDeclRefExprLHS2, anything()))),
+                        hasRHS(DirectDeclRefExprRHS1)),
+
+                  allOf(hasLHS(allOf(NoIndirectDeclRefExpr2,
+                                     anyOf(DirectDeclRefExprLHS1, anything()))),
+                        hasRHS(DirectDeclRefExprRHS2))))))));
+
+  Finder->addMatcher(
+      // We match a declaration statement,
+      declStmt(
+          stmt().bind("declStmt1"),
+
+          // which contains a single variable declaration,
+          HasVarDecl1,
+
+          // and which has a successor,
+          matchers::hasSuccessor(anyOf(
+              // which is another declaration statement,
+              declStmt(stmt().bind("declStmt2"),
+
+                       // which contains a single variable declaration,
+                       HasVarDecl2,
+
+                       // and which has a successor which is a return statement
+                       // which may contain var decl 1 or 2.
+                       matchers::hasSuccessor(ReturnStmt2)),
+              // or which is a return statement only containing var decl 1.
+              ReturnStmt1))),
+      this);
+}
+
+void UnnecessaryIntermediateVarCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "MaximumLineLength", MaximumLineLength);
+}
+
+void UnnecessaryIntermediateVarCheck::emitMainWarning(const VarDecl *VarDecl1,
+                                                      const VarDecl *VarDecl2) {
+  diag(VarDecl1->getLocation(), "unnecessary intermediate variable %0",
+       DiagnosticIDs::Warning)
+      << VarDecl1;
+
+  if (VarDecl2)
+    diag(VarDecl2->getLocation(), "and so is %0", DiagnosticIDs::Warning)
+        << VarDecl2;
+}
+
+void UnnecessaryIntermediateVarCheck::emitUsageInComparisonNote(
+    const DeclRefExpr *VarRef, const bool IsPlural) {
+  diag(VarRef->getLocation(),
+       "because %select{they are|it is}0 only used when returning the result of this comparison",
+       DiagnosticIDs::Note)
+      << (IsPlural ? 0 : 1);
+}
+
+void UnnecessaryIntermediateVarCheck::emitVarDeclRemovalNote(
+    const VarDecl *VarDecl1, const DeclStmt *DeclStmt1, const VarDecl *VarDecl2,
+    const DeclStmt *DeclStmt2) {
+  diag(VarDecl1->getLocation(), "consider removing %select{both this|the}0 variable declaration",
+       DiagnosticIDs::Note)
+      << ((VarDecl2 && DeclStmt2) ? 0 : 1)
+      << FixItHint::CreateRemoval(DeclStmt1->getSourceRange());
+
+  if (VarDecl2 && DeclStmt2)
+    diag(VarDecl2->getLocation(), "and this one", DiagnosticIDs::Note)
+        << FixItHint::CreateRemoval(DeclStmt2->getSourceRange());
+}
+
+void UnnecessaryIntermediateVarCheck::emitReturnReplacementNote(
+    const Expr *LHS, const StringRef LHSReplacement, const Expr *RHS,
+    const StringRef RHSReplacement, const BinaryOperator *BinOpToReverse) {
+  auto Diag =
+      diag(LHS->getLocStart(),
+           "and directly using the variable initialization expression%select{s|}0 here",
+           DiagnosticIDs::Note)
+      << ((isa<DeclRefExpr>(LHS) && RHS && isa<DeclRefExpr>(RHS)) ? 0 : 1)
+      << FixItHint::CreateReplacement(LHS->getSourceRange(), LHSReplacement);
+
+  if (RHS)
+    Diag << FixItHint::CreateReplacement(RHS->getSourceRange(), RHSReplacement);
+
+  if (BinOpToReverse) {
+    const StringRef ReversedBinOpText = BinaryOperator::getOpcodeStr(
+        BinaryOperator::reverseComparisonOp(BinOpToReverse->getOpcode()));
+
+    Diag << FixItHint::CreateReplacement(
+        SourceRange(BinOpToReverse->getOperatorLoc(),
+                    BinOpToReverse->getOperatorLoc().getLocWithOffset(
+                        BinOpToReverse->getOpcodeStr().size())),
+        ReversedBinOpText);
+  }
+}
+
+void UnnecessaryIntermediateVarCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *DeclarationStmt1 =
+      Result.Nodes.getNodeAs<DeclStmt>("declStmt1");
+  const auto *Init1 = Result.Nodes.getNodeAs<Expr>("init1");
+  const auto *VariableDecl1 = Result.Nodes.getNodeAs<VarDecl>("varDecl1");
+  const auto *VarRefLHS1 =
+      Result.Nodes.getNodeAs<DeclRefExpr>("declRefExprLHS1");
+  const auto *VarRefRHS1 =
+      Result.Nodes.getNodeAs<DeclRefExpr>("declRefExprRHS1");
+
+  // If we already checked this declaration in a 2-decl match, skip it.
+  if (CheckedDeclStmt.count(DeclarationStmt1))
+    return;
+
+  const auto *DeclarationStmt2 =
+      Result.Nodes.getNodeAs<DeclStmt>("declStmt2");
+  const auto *Init2 = Result.Nodes.getNodeAs<Expr>("init2");
+  const auto *VariableDecl2 = Result.Nodes.getNodeAs<VarDecl>("varDecl2");
+  const auto *VarRefLHS2 =
+      Result.Nodes.getNodeAs<DeclRefExpr>("declRefExprLHS2");
+  const auto *VarRefRHS2 =
+      Result.Nodes.getNodeAs<DeclRefExpr>("declRefExprRHS2");
+
+  // Add the second declaration to the cache to make sure it doesn't get
+  // matches individually afterwards.
+  CheckedDeclStmt.insert(DeclarationStmt2);
+
+  const auto *Return1 = Result.Nodes.getNodeAs<ReturnStmt>("returnStmt1");
+  const auto *Return2 = Result.Nodes.getNodeAs<ReturnStmt>("returnStmt2");
+
+  const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binOp");
+
+  if (Return1) {
+    // This is the case where we only have one variable declaration before the
+    // return statement.
+
+    // First we get the source code of the initializer expression of the
+    // variable declaration.
+    auto Init1Text =
+        clang::tooling::fixit::getText(*Init1, *Result.Context).str();
+    if (Init1Text.empty())
+      return;
+
+    // If the expression is a binary operator, we wrap it in parentheses to keep
+    // the same operator precendence.
+    if (isa<BinaryOperator>(Init1->IgnoreImplicit())) {
+      Init1Text = "(" + Init1Text + ")";
+    }
+
+    // Next we compute the return indentation length and the return length to be
+    // able to know what length the return statement will have once the fixes
+    // are applied.
+    const int ReturnIndentTextLength =
+        Lexer::getIndentationForLine(Return1->getLocStart(),
+                                     Result.Context->getSourceManager())
+            .size();
+
+    const int ReturnTextLength =
+        clang::tooling::fixit::getText(*Return1,
+                                       *Result.Context)
+            .size();
+
+    const int NewReturnLength = ReturnIndentTextLength + ReturnTextLength -
+                                VariableDecl1->getName().size() +
+                                Init1Text.size()
+                                + 1; // For the trailing semicolon
+
+    // If the new length is over the statement limit, then folding the
+    // expression wouldn't really benefit readability. Therefore we abort.
+    if (NewReturnLength > MaximumLineLength)
+      return;
+
+    // Otherwise, we're all good and we emit the diagnostics along with the fix
+    // it hints.
+
+    emitMainWarning(VariableDecl1);
+
+    if (VarRefLHS1) {
+      emitUsageInComparisonNote(VarRefLHS1, false);
+      emitVarDeclRemovalNote(VariableDecl1, DeclarationStmt1);
+      emitReturnReplacementNote(VarRefLHS1, Init1Text);
+    } else if (VarRefRHS1) {
+      // If the variable is on the RHS of the comparison, we need to reverse the
+      // operands of the binary operator to keep the same execution order.
+      StringRef LHSText = clang::tooling::fixit::getText(
+          *BinOp->getLHS(), *Result.Context);
+      if (LHSText.empty())
+        return;
+
+      emitUsageInComparisonNote(VarRefRHS1, false);
+      emitVarDeclRemovalNote(VariableDecl1, DeclarationStmt1);
+      emitReturnReplacementNote(BinOp->getLHS(), Init1Text, VarRefRHS1,
+                                LHSText, BinOp);
+    } else
+      return;
+  } else if (Return2) {
+    // This is the case where there are two variable declarations before the
+    // return statement.
+    const bool HasVarRef1 = VarRefLHS1 || VarRefRHS1;
+    const bool HasVarRef2 = VarRefLHS2 || VarRefRHS2;
+
+    // First we get the source code of the initializer expressions of the
+    // variable declarations.
+    std::string Init1Text =
+        clang::tooling::fixit::getText(*Init1, *Result.Context).str();
+    if (Init1Text.empty())
+      return;
+
+    std::string Init2Text =
+        clang::tooling::fixit::getText(*Init2, *Result.Context).str();
+    if (Init2Text.empty())
+      return;
+
+    // If the expressiond are binary operators, we wrap them in parentheses to
+    // keep the same operator precendence.
+    if (isa<BinaryOperator>(Init1->IgnoreImplicit()))
+      Init1Text = "(" + Init1Text + ")";
+
+    if (isa<BinaryOperator>(Init2->IgnoreImplicit()))
+      Init2Text = "(" + Init2Text + ")";
+
+    // Next we compute the return indentation length and the return length to be
+    // able to know what length the return statement will have once the fixes
+    // are applied.
+    const int ReturnIndentTextLength =
+        Lexer::getIndentationForLine(Return2->getLocStart(),
+                                     Result.Context->getSourceManager())
+            .size();
+
+    const int ReturnTextLength =
+        clang::tooling::fixit::getText(*Return2,
+                                       *Result.Context)
+            .size();
+
+    auto NewReturnLength = ReturnIndentTextLength + ReturnTextLength
+                           + 1; // For the trailing semicolon
+
+    if (HasVarRef1) {
+      NewReturnLength -= VariableDecl1->getName().size();
+      NewReturnLength += Init1Text.size();
+    }
+
+    if (HasVarRef2) {
+      NewReturnLength -= VariableDecl2->getName().size();
+      NewReturnLength += Init2Text.size();
+    }
+
+    // If the new length is over the statement limit, then folding the
+    // expression wouldn't really benefit readability. Therefore we abort.
+    if (NewReturnLength > MaximumLineLength)
+      return;
+
+    // Otherwise, we're all good and we emit the diagnostics along with the fix
+    // it hints.
+
+    if (HasVarRef1 && HasVarRef2)
+      emitMainWarning(VariableDecl1, VariableDecl2);
+    else if (HasVarRef1)
+      emitMainWarning(VariableDecl1);
+    else if (HasVarRef2)
+      emitMainWarning(VariableDecl2);
+
+    if (VarRefLHS1 && VarRefRHS2) {
+      emitUsageInComparisonNote(VarRefLHS1, true);
+      emitVarDeclRemovalNote(VariableDecl1, DeclarationStmt1, VariableDecl2,
+                             DeclarationStmt2);
+      emitReturnReplacementNote(VarRefLHS1, Init1Text, VarRefRHS2, Init2Text);
+    } else if (VarRefLHS2 && VarRefRHS1) {
+      emitUsageInComparisonNote(VarRefLHS2, true);
+      emitVarDeclRemovalNote(VariableDecl1, DeclarationStmt1, VariableDecl2,
+                             DeclarationStmt2);
+      // Here we reverse the operands because we want to keep the same execution
+      // order.
+      emitReturnReplacementNote(VarRefLHS2, Init1Text, VarRefRHS1, Init2Text,
+                                BinOp);
+    } else if (VarRefLHS1 && !VarRefRHS2) {
+      emitUsageInComparisonNote(VarRefLHS1, false);
+      emitVarDeclRemovalNote(VariableDecl1, DeclarationStmt1);
+      emitReturnReplacementNote(VarRefLHS1, Init1Text);
+    } else if (!VarRefLHS1 && VarRefRHS2) {
+      // If the variable is on the RHS of the comparison, we need to reverse the
+      // operands of the binary operator to keep the same execution order.
+      StringRef LHSText = clang::tooling::fixit::getText(
+          *BinOp->getLHS(), *Result.Context);
+      if (LHSText.empty())
+        return;
+
+      emitUsageInComparisonNote(VarRefRHS2, false);
+      emitVarDeclRemovalNote(VariableDecl2, DeclarationStmt2);
+      emitReturnReplacementNote(BinOp->getLHS(), Init2Text, VarRefRHS2,
+                                LHSText, BinOp);
+    } else if (VarRefLHS2 && !VarRefRHS1) {
+      emitUsageInComparisonNote(VarRefLHS2, false);
+      emitVarDeclRemovalNote(VariableDecl2, DeclarationStmt2);
+      emitReturnReplacementNote(VarRefLHS2, Init2Text);
+    } else if (!VarRefLHS2 && VarRefRHS1) {
+      // If the variable is on the RHS of the comparison, we need to reverse the
+      // operands of the binary operator to keep the same execution order.
+      StringRef LHSText = clang::tooling::fixit::getText(
+          *BinOp->getLHS(), *Result.Context);
+      if (LHSText.empty())
+        return;
+
+      emitUsageInComparisonNote(VarRefRHS1, false);
+      emitVarDeclRemovalNote(VariableDecl1, DeclarationStmt1);
+      emitReturnReplacementNote(BinOp->getLHS(), Init1Text, VarRefRHS1,
+                                LHSText, BinOp);
+    }
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New module `abseil` for checks related to the `Abseil <https://abseil.io>`_
   library.
 
 - New module ``portability``.
 
 - New module ``zircon`` for checks related to Fuchsia's Zircon kernel.
 
 - New :doc:`bugprone-throw-keyword-missing
   <clang-tidy/checks/bugprone-throw-keyword-missing>` check
 
   Diagnoses when a temporary object that appears to be an exception is
   constructed but not thrown.
 
 - New :doc:`bugprone-unused-return-value
- New :doc:`modernize-use-uncaught-exceptions
   ``std::uncaught_exceptions``.
 
 - New :doc:`portability-simd-intrinsics
   <clang-tidy/checks/portability-simd-intrinsics>` check
 
   Warns or suggests alternatives if SIMD intrinsics are used which can be replaced by
   ``std::experimental::simd`` operations.
 
+- New :doc:`readability-unnecessary-intermediate-var
+  <clang-tidy/checks/readability-unnecessary-intermediate-var>`_ check
+
+  Detects unnecessary intermediate variables before ``return``
+  statements that return the result of a simple comparison. This check also
+  suggests to directly inline the initializer expression of the variable
+  declaration into the ``return`` expression.
+
 - New :doc:`zircon-temporary-objects
   <clang-tidy/checks/zircon-temporary-objects>` check
 
   Warns on construction of specific temporary objects in the Zircon kernel.
 
 - New alias :doc:`hicpp-avoid-goto
   <clang-tidy/checks/hicpp-avoid-goto>` to :doc:`cppcoreguidelines-avoid-goto
   <clang-tidy/checks/cppcoreguidelines-avoid-goto>`
   added.
 
 - The 'misc-forwarding-reference-overload' check was renamed to :doc:`bugprone-forwarding-reference-overload
   <clang-tidy/checks/bugprone-forwarding-reference-overload>`
 
 - The 'misc-incorrect-roundings' check was renamed to :doc:`bugprone-incorrect-roundings
   <clang-tidy/checks/bugprone-incorrect-roundings>`
 
 - The 'misc-lambda-function-name' check was renamed to :doc:`bugprone-lambda-function-name
   <clang-tidy/checks/bugprone-lambda-function-name>`
 
 - The 'misc-macro-parentheses' check was renamed to :doc:`bugprone-macro-parentheses
   <clang-tidy/checks/bugprone-macro-parentheses>`
 

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-accessed-through-instance
    readability-static-definition-in-anonymous-namespace
    readability-string-compare
    readability-uniqueptr-delete-release
+   readability-unnecessary-intermediate-var
    zircon-temporary-objects

docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst

+.. title:: clang-tidy - readability-unnecessary-intermediate-var
+
+readability-unnecessary-intermediate-var
+========================================
+
+Detects unnecessary intermediate variables before ``return` statements returning the
+result of a simple comparison. This check also suggests to directly inline the
+initializer expression of the variable declaration into the ``return` expression.
+
+Example:
+.. code-block:: c++
+
+  // the checker detects
+
+  auto test = foo();
+  return (test == MY_CONST);
+
+  // and suggests to fix it into
+
+  return (foo() == MY_CONST);

test/clang-tidy/readability-unnecessary-intermediate-var.cpp

+// 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-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return (foo() == 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 f_lambda() {
+  auto test = []() { return 1; } (); // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == 1);
+  // CHECK-FIXES: {{^}}  return ([]() { 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
+}
+
+template <typename T>
+bool f_template() {
+  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 f_operator_inversion() {
+  auto test1 = 1; // Test1
+  // CHECK-FIXES: {{^}}  // Test1{{$}}
+  return (2 > test1);
+  // CHECK-FIXES: {{^}}  return (1 < 2);{{$}}
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var]
+  // CHECK-MESSAGES: :[[@LINE-3]]:15: 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 f_init2_contains_var1() {
+  auto test1 = 1; // Test1
+  auto test2 = test1; // Test2
+  // CHECK-FIXES: {{^}}  // Test2{{$}}
+  return (2 == test2);
+  // CHECK-FIXES: {{^}}  return (test1 == 2);{{$}}
+  // 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
+}
+
+bool f_double_use() {
+  auto test = 1;
+  return (test == (test + 1));
+}
+
+bool f_double_use2() {
+  auto test1 = 1;
+  auto test2 = 2;
+  return (test1 == (test1 + 1));
+}
+
+bool f_double_use3() {
+  auto test1 = 1;
+  auto test2 = 2;
+  return (test2 == (test2 + 1));
+}
+
+bool f_double_use4() {
+  auto test1 = 1;
+  auto test2 = 2;
+  return ((test1 + 1) == test1);
+}
+
+bool f_double_use5() {
+  auto test1 = 1;
+  auto test2 = 2;
+  return ((test2 + 1) == test2);
+}
+
+bool f_intermediate_statement() {
+  auto test = 1;
+  test = 2;
+  return (test == 1);
+}
+
+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 == "");
+}
+
+bool f_expression_at_exact_max_line_length() {
+  auto test = "this is an expression which gives the maximum line length..."; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == "");
+  // CHECK-FIXES: {{^}}  return ("this is an expression which gives the maximum line length..." == "");{{$}}
+  // 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 f_expression_just_under_max_line_length() {
+  auto test = "this is an expression just under the maximum line length..."; // Test
+  // CHECK-FIXES: {{^}}  // Test{{$}}
+  return (test == "");
+  // CHECK-FIXES: {{^}}  return ("this is an expression just under the maximum line length..." == "");{{$}}
+  // 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 f_expression_just_above_max_line_length() {
+  auto test = "this is an expression just above the maximum line length....."; // Test
+  return (test == "");
+
+}
+
+bool f_preprocessor_macro1() {
+  auto test = 1; // Test
+#ifdef INTERMITTENT_MACRO
+  return (test == 1);
+#endif
+}
+
+bool f_preprocessor_macro2() {
+#ifdef INTERMITTENT_MACRO
+  auto test = 1; // Test
+  return (test == 1);
+#endif
+}
+
+#define INTERMITTENT_MACRO
+bool f_preprocessor_macro3() {
+#ifdef INTERMITTENT_MACRO
+  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
+#endif
+}
+
+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_macro4() {
+  auto test = some_func();
+  assert(test); // <- should not be removed regardless of whether NDEBUG is set or not
+  return test;
+}
 No newline at end of file

unittests/clang-tidy/ReadabilityModuleTest.cpp

 #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 {
 namespace tidy {
 namespace test {
 
 using readability::BracesAroundStatementsCheck;
 using readability::NamespaceCommentCheck;
TEST(BracesAroundStatementsCheckTest, ImplicitCastInReturn) {
   EXPECT_EQ("const char *f() {\n"
             "  if (true) { return \"\";\n"
             "}\n"
             "  return \"abc\";\n"
             "}\n",
             runCheckOnCode<BracesAroundStatementsCheck>(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