Index: clang-tidy/readability/CMakeLists.txt =================================================================== --- clang-tidy/readability/CMakeLists.txt +++ clang-tidy/readability/CMakeLists.txt @@ -28,6 +28,7 @@ StaticAccessedThroughInstanceCheck.cpp StaticDefinitionInAnonymousNamespaceCheck.cpp UniqueptrDeleteReleaseCheck.cpp + UnnecessaryIntermediateVarCheck.cpp LINK_LIBS clangAST Index: clang-tidy/readability/ReadabilityTidyModule.cpp =================================================================== --- clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tidy/readability/ReadabilityTidyModule.cpp @@ -35,6 +35,7 @@ #include "StaticAccessedThroughInstanceCheck.h" #include "StaticDefinitionInAnonymousNamespaceCheck.h" #include "UniqueptrDeleteReleaseCheck.h" +#include "UnnecessaryIntermediateVarCheck.h" namespace clang { namespace tidy { @@ -93,6 +94,8 @@ "readability-simplify-boolean-expr"); CheckFactories.registerCheck( "readability-uniqueptr-delete-release"); + CheckFactories.registerCheck( + "readability-unnecessary-intermediate-var"); } }; Index: clang-tidy/readability/UnnecessaryIntermediateVarCheck.h =================================================================== --- /dev/null +++ clang-tidy/readability/UnnecessaryIntermediateVarCheck.h @@ -0,0 +1,59 @@ +//===--- 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 "llvm/ADT/SmallSet.h" + +namespace clang { +namespace tidy { +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) + : ClangTidyCheck(Name, Context), + MaximumLineLength(Options.get("MaximumLineLength", 100)) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + 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 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,444 @@ +//===--- 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/LexerUtils.h" +#include "../utils/Matchers.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +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")))); + + const auto DirectDeclRefExprRHS1 = + // We match a direct declaration reference expression pointing + // to the variable declaration 1 as RHS. + ignoringImplicit(ignoringParenCasts(declRefExpr( + to(equalsBoundNode("varDecl1")), expr().bind("declRefExprRHS1")))); + + const auto NoIndirectDeclRefExpr1 = + // We match a declaration reference expression in any descendant + // pointing to variable declaration 1. + unless(hasDescendant(declRefExpr(to(equalsBoundNode("varDecl1"))))); + + const auto DirectDeclRefExprLHS2 = + // We match a direct declaration reference expression pointing + // to the variable declaration 2 as LHS. + ignoringImplicit(ignoringParenCasts(declRefExpr( + to(equalsBoundNode("varDecl2")), expr().bind("declRefExprLHS2")))); + + const auto DirectDeclRefExprRHS2 = + // We match a direct declaration reference expression pointing + // to the variable declaration 2 as RHS. + ignoringImplicit(ignoringParenCasts(declRefExpr( + to(equalsBoundNode("varDecl2")), expr().bind("declRefExprRHS2")))); + + const auto NoIndirectDeclRefExpr2 = + // We match a declaration reference expression in any descendant + // pointing to variable declaration 2. + 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(LHS) && RHS && isa(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("declStmt1"); + const Expr *Init1 = Result.Nodes.getNodeAs("init1"); + const VarDecl *VariableDecl1 = Result.Nodes.getNodeAs("varDecl1"); + const DeclRefExpr *VarRefLHS1 = + Result.Nodes.getNodeAs("declRefExprLHS1"); + const DeclRefExpr *VarRefRHS1 = + Result.Nodes.getNodeAs("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("declStmt2"); + const Expr *Init2 = Result.Nodes.getNodeAs("init2"); + const VarDecl *VariableDecl2 = Result.Nodes.getNodeAs("varDecl2"); + const DeclRefExpr *VarRefLHS2 = + Result.Nodes.getNodeAs("declRefExprLHS2"); + const DeclRefExpr *VarRefRHS2 = + Result.Nodes.getNodeAs("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("returnStmt1"); + const ReturnStmt *Return2 = Result.Nodes.getNodeAs("returnStmt2"); + + const BinaryOperator *BinOp = Result.Nodes.getNodeAs("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. + const auto Init1TextOpt = + utils::lexer::getStmtText(Init1, Result.Context->getSourceManager()); + if (!Init1TextOpt) { + return; + } + auto Init1Text = (*Init1TextOpt).str(); + + // If the expression is a binary operator, we wrap it in parentheses to keep + // the same operator precendence. + if (isa(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 = + (*utils::lexer::getStmtText(Return1, + Result.Context->getSourceManager())) + .size(); + + const auto NewReturnLength = ReturnIndentTextLength + ReturnTextLength - + VariableDecl1->getName().size() + + Init1Text.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. + + 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. + const auto LHSTextOpt = utils::lexer::getStmtText( + BinOp->getLHS(), Result.Context->getSourceManager()); + if (!LHSTextOpt) { + return; + } + + emitUsageInComparisonNote(VarRefRHS1, false); + emitVarDeclRemovalNote(VariableDecl1, DeclarationStmt1); + emitReturnReplacementNote(BinOp->getLHS(), Init1Text, VarRefRHS1, + *LHSTextOpt, 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. + const auto Init1TextOpt = + utils::lexer::getStmtText(Init1, Result.Context->getSourceManager()); + if (!Init1TextOpt) { + return; + } + auto Init1Text = (*Init1TextOpt).str(); + + const auto Init2TextOpt = + utils::lexer::getStmtText(Init2, Result.Context->getSourceManager()); + if (!Init2TextOpt) { + return; + } + auto Init2Text = (*Init2TextOpt).str(); + + // If the expressiond are binary operators, we wrap them in parentheses to + // keep the same operator precendence. + if (isa(Init1->IgnoreImplicit())) { + Init1Text = "(" + Init1Text + ")"; + } + + if (isa(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 = + (*utils::lexer::getStmtText(Return2, + Result.Context->getSourceManager())) + .size(); + + auto NewReturnLength = ReturnIndentTextLength + ReturnTextLength; + + 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. + const auto LHSTextOpt = utils::lexer::getStmtText( + BinOp->getLHS(), Result.Context->getSourceManager()); + if (!LHSTextOpt) { + return; + } + + emitUsageInComparisonNote(VarRefRHS2, false); + emitVarDeclRemovalNote(VariableDecl2, DeclarationStmt2); + emitReturnReplacementNote(BinOp->getLHS(), Init2Text, VarRefRHS2, + *LHSTextOpt, 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. + const auto LHSTextOpt = utils::lexer::getStmtText( + BinOp->getLHS(), Result.Context->getSourceManager()); + if (!LHSTextOpt) { + return; + } + + emitUsageInComparisonNote(VarRefRHS1, false); + emitVarDeclRemovalNote(VariableDecl1, DeclarationStmt1); + emitReturnReplacementNote(BinOp->getLHS(), Init1Text, VarRefRHS1, + *LHSTextOpt, BinOp); + } + } +} + +} // namespace readability +} // namespace tidy +} // namespace clang Index: clang-tidy/utils/LexerUtils.h =================================================================== --- clang-tidy/utils/LexerUtils.h +++ clang-tidy/utils/LexerUtils.h @@ -22,6 +22,9 @@ Token getPreviousToken(const ASTContext &Context, SourceLocation Location, bool SkipComments = true); +/// Get source code text for statement. +Optional getStmtText(const Stmt* Statement, const SourceManager& SM); + } // namespace lexer } // namespace utils } // namespace tidy Index: clang-tidy/utils/LexerUtils.cpp =================================================================== --- clang-tidy/utils/LexerUtils.cpp +++ clang-tidy/utils/LexerUtils.cpp @@ -35,6 +35,16 @@ return Token; } +Optional getStmtText(const Stmt* Statement, const SourceManager& SM) { + bool Error = false; + auto Ret = Lexer::getSourceText( + CharSourceRange::getTokenRange(Statement->getSourceRange()), + SM, LangOptions(), + &Error); + + return Error ? llvm::NoneType() : Optional(Ret); +} + } // namespace lexer } // namespace utils } // namespace tidy Index: clang-tidy/utils/Matchers.h =================================================================== --- clang-tidy/utils/Matchers.h +++ clang-tidy/utils/Matchers.h @@ -12,6 +12,8 @@ #include "TypeTraits.h" #include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Analysis/CFG.h" namespace clang { namespace tidy { @@ -44,6 +46,67 @@ return referenceType(pointee(qualType(isConstQualified()))); } +// Matches the next statement within the parent statement sequence. +AST_MATCHER_P(Stmt, hasSuccessor, + ast_matchers::internal::Matcher, InnerMatcher) { + using namespace ast_matchers; + + // We get the first parent, making sure that we're not in a case statement + // not in a compound statement directly inside a switch, because this causes + // the buildCFG call to crash. + auto Parent = selectFirst( + "parent", + match( + stmt(hasAncestor(stmt( + unless(caseStmt()), + unless(compoundStmt(hasParent(switchStmt()))), + stmt().bind("parent")))), + Node, Finder->getASTContext())); + + // We build a Control Flow Graph (CFG) from the parent statement. + std::unique_ptr StatementCFG = + CFG::buildCFG(nullptr, const_cast(Parent), &Finder->getASTContext(), + CFG::BuildOptions()); + + if (!StatementCFG) { + return false; + } + + // We iterate through all the CFGBlocks, which basically means that we go over + // all the possible branches of the code and therefore cover all statements. + for (auto& Block : *StatementCFG) { + if (!Block) { + continue; + } + + // We iterate through all the statements of the block. + bool ReturnNextStmt = false; + for (auto& BlockItem : *Block) { + Optional CFGStatement = BlockItem.getAs(); + if (!CFGStatement) { + if (ReturnNextStmt) { + return false; + } + + continue; + } + + // If we found the next statement, we apply the inner matcher and return + // the result. + if (ReturnNextStmt) { + return InnerMatcher.matches(*CFGStatement->getStmt(), Finder, Builder); + } + + if (CFGStatement->getStmt() == &Node) { + ReturnNextStmt = true; + } + } + } + + // If we didn't find a successor, we just return false. + return false; +} + } // namespace matchers } // namespace tidy } // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -132,6 +132,14 @@ Finds member expressions that access static members through instances and replaces them with uses of the appropriate qualified-id. +- New `readability-unnecessary-intermediate-var + `_ 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. + - Added `modernize-use-emplace.IgnoreImplicitConstructors `_ option. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -193,3 +193,4 @@ readability-static-accessed-through-instance readability-static-definition-in-anonymous-namespace readability-uniqueptr-delete-release + readability-unnecessary-intermediate-var 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: test/clang-tidy/readability-unnecessary-intermediate-var.cpp =================================================================== --- /dev/null +++ test/clang-tidy/readability-unnecessary-intermediate-var.cpp @@ -0,0 +1,174 @@ +// RUN: %check_clang_tidy %s readability-unnecessary-intermediate-var %t + +bool f() { + auto test = 1; // Test + // CHECK-FIXES: {{^}} // Test{{$}} + return (test == 1); + // CHECK-FIXES: {{^}} return (1 == 1);{{$}} + // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test' [readability-unnecessary-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration + // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here +} + +bool f2() { + auto test1 = 1; // Test1 + // CHECK-FIXES: {{^}} // Test1{{$}} + auto test2 = 2; // Test2 + // CHECK-FIXES: {{^}} // Test2{{$}} + return (test1 == test2); + // CHECK-FIXES: {{^}} return (1 == 2);{{$}} + // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-unnecessary-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration + // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one + // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here +} + +bool f3() { + auto test1 = 1; // Test1 + // CHECK-FIXES: {{^}} // Test1{{$}} + auto test2 = 2; // Test2 + return (test1 == 2); + // CHECK-FIXES: {{^}} return (1 == 2);{{$}} + // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration + // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here +} + +bool f4() { + auto test1 = 1; // Test1 + auto test2 = 2; // Test2 + // CHECK-FIXES: {{^}} // Test2{{$}} + return (test2 == 3); + // CHECK-FIXES: {{^}} return (2 == 3);{{$}} + // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration + // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here +} + +bool f5() { + auto test1 = 1; // Test1 + // CHECK-FIXES: {{^}} // Test1{{$}} + auto test2 = 2; // Test2 + return (2 == test1); + // CHECK-FIXES: {{^}} return (1 == 2);{{$}} + // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: unnecessary intermediate variable 'test1' [readability-unnecessary-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration + // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here +} + +bool f6() { + auto test1 = 1; // Test1 + auto test2 = 2; // Test2 + // CHECK-FIXES: {{^}} // Test2{{$}} + return (3 == test2); + // CHECK-FIXES: {{^}} return (2 == 3);{{$}} + // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: unnecessary intermediate variable 'test2' [readability-unnecessary-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration + // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here +} + +int foo() { return 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 +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 == ""); +}