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<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
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-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
Index: docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/readability-unnecessary-intermediate-var.rst
@@ -0,0 +1,20 @@
+.. 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 checker 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);
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -224,3 +224,4 @@
readability-static-definition-in-anonymous-namespace
readability-string-compare
readability-uniqueptr-delete-release
+ readability-unnecessary-intermediate-var
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -107,6 +107,14 @@
<http://clang.llvm.org/extra/clang-tidy/checks/hicpp-avoid-goto.html>`_ to
`cppcoreguidelines-avoid-goto <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_
added.
+
+- New `readability-unnecessary-intermediate-var
+ <http://clang.llvm.org/extra/clang-tidy/checks/readability-unnecessary-intermediate-var.html>`_ check
+
+ This new 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.
- The 'misc-forwarding-reference-overload' check was renamed to `bugprone-forwarding-reference-overload
<http://clang.llvm.org/extra/clang-tidy/checks/bugprone-forwarding-reference-overload.html>`_
Index: clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/readability/UnnecessaryIntermediateVarCheck.h
@@ -0,0 +1,121 @@
+//===--- 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 (auto &Block : *StatementCFG) {
+ if (!Block)
+ continue;
+
+ // We iterate through all the statements of the block.
+ bool ReturnNextStmt = false;
+ for (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
Index: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp
@@ -0,0 +1,459 @@
+//===--- 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"
+#include "clang/Analysis/CFG.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+UnnecessaryIntermediateVarCheck::UnnecessaryIntermediateVarCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {
+ auto File = Context->getCurrentFile();
+ auto Style = format::getStyle(*Context->getOptionsForFile(File).FormatStyle,
+ File, "none");
+ auto DefaultMaximumLineLength = 80;
+
+ if (!Style) {
+ DefaultMaximumLineLength = (*Style).ColumnLimit;
+ }
+
+ MaximumLineLength = Options.get("MaximumLineLength", (*Style).ColumnLimit);
+}
+
+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 %0 only used when returning the result of this comparison",
+ DiagnosticIDs::Note)
+ << (IsPlural ? "they are" : "it is");
+}
+
+void UnnecessaryIntermediateVarCheck::emitVarDeclRemovalNote(
+ const VarDecl *VarDecl1, const DeclStmt *DeclStmt1, const VarDecl *VarDecl2,
+ const DeclStmt *DeclStmt2) {
+ diag(VarDecl1->getLocation(), "consider removing %0 variable declaration",
+ DiagnosticIDs::Note)
+ << ((VarDecl2 && DeclStmt2) ? "both this" : "the")
+ << 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%0 here",
+ DiagnosticIDs::Note)
+ << ((isa<DeclRefExpr>(LHS) && RHS && isa<DeclRefExpr>(RHS)) ? "s" : "")
+ << FixItHint::CreateReplacement(LHS->getSourceRange(), LHSReplacement);
+
+ if (RHS) {
+ Diag << FixItHint::CreateReplacement(RHS->getSourceRange(), RHSReplacement);
+ }
+
+ if (BinOpToReverse) {
+ const auto 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 DeclStmt *DeclarationStmt1 =
+ Result.Nodes.getNodeAs<DeclStmt>("declStmt1");
+ const Expr *Init1 = Result.Nodes.getNodeAs<Expr>("init1");
+ const VarDecl *VariableDecl1 = Result.Nodes.getNodeAs<VarDecl>("varDecl1");
+ const DeclRefExpr *VarRefLHS1 =
+ Result.Nodes.getNodeAs<DeclRefExpr>("declRefExprLHS1");
+ const DeclRefExpr *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 DeclStmt *DeclarationStmt2 =
+ Result.Nodes.getNodeAs<DeclStmt>("declStmt2");
+ const Expr *Init2 = Result.Nodes.getNodeAs<Expr>("init2");
+ const VarDecl *VariableDecl2 = Result.Nodes.getNodeAs<VarDecl>("varDecl2");
+ const DeclRefExpr *VarRefLHS2 =
+ Result.Nodes.getNodeAs<DeclRefExpr>("declRefExprLHS2");
+ const DeclRefExpr *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 ReturnStmt *Return1 = Result.Nodes.getNodeAs<ReturnStmt>("returnStmt1");
+ const ReturnStmt *Return2 = Result.Nodes.getNodeAs<ReturnStmt>("returnStmt2");
+
+ const BinaryOperator *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 auto ReturnIndentTextLength =
+ Lexer::getIndentationForLine(Return1->getLocStart(),
+ Result.Context->getSourceManager())
+ .size();
+
+ const auto ReturnTextLength =
+ clang::tooling::fixit::getText(*Return1,
+ *Result.Context)
+ .size();
+
+ const auto 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.
+ auto 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.
+ auto Init1Text =
+ clang::tooling::fixit::getText(*Init1, *Result.Context).str();
+ if (Init1Text.empty()) {
+ return;
+ }
+
+ auto 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 auto ReturnIndentTextLength =
+ Lexer::getIndentationForLine(Return2->getLocStart(),
+ Result.Context->getSourceManager())
+ .size();
+
+ const auto 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.
+ auto 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.
+ auto 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
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===================================================================
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -36,6 +36,7 @@
#include "StaticDefinitionInAnonymousNamespaceCheck.h"
#include "StringCompareCheck.h"
#include "UniqueptrDeleteReleaseCheck.h"
+#include "UnnecessaryIntermediateVarCheck.h"
namespace clang {
namespace tidy {
@@ -96,6 +97,8 @@
"readability-simplify-boolean-expr");
CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(
"readability-uniqueptr-delete-release");
+ CheckFactories.registerCheck<UnnecessaryIntermediateVarCheck>(
+ "readability-unnecessary-intermediate-var");
}
};
Index: clang-tidy/readability/CMakeLists.txt
===================================================================
--- clang-tidy/readability/CMakeLists.txt
+++ clang-tidy/readability/CMakeLists.txt
@@ -29,6 +29,7 @@
StaticDefinitionInAnonymousNamespaceCheck.cpp
StringCompareCheck.cpp
UniqueptrDeleteReleaseCheck.cpp
+ UnnecessaryIntermediateVarCheck.cpp
LINK_LIBS
clangAST
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
