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 + UselessIntermediateVarCheck.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 "UselessIntermediateVarCheck.h" namespace clang { namespace tidy { @@ -75,6 +76,8 @@ "readability-static-accessed-through-instance"); CheckFactories.registerCheck( "readability-static-definition-in-anonymous-namespace"); + CheckFactories.registerCheck( + "readability-useless-intermediate-var"); CheckFactories.registerCheck( "readability-named-parameter"); CheckFactories.registerCheck( Index: clang-tidy/readability/UselessIntermediateVarCheck.h =================================================================== --- /dev/null +++ clang-tidy/readability/UselessIntermediateVarCheck.h @@ -0,0 +1,54 @@ +//===--- UselessIntermediateVarCheck.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_USELESS_INTERMEDIATE_VAR_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USELESS_INTERMEDIATE_VAR_H + +#include +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// This checker detects useless 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-useless-intermediate-var.html +class UselessIntermediateVarCheck : public ClangTidyCheck { +public: + UselessIntermediateVarCheck(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, bool IsPlural); + void emitVarDeclRemovalNote(const VarDecl* VarDecl1, const DeclStmt* DeclStmt1, + const VarDecl* VarDecl2 = nullptr, + const DeclStmt* DeclStmt2 = nullptr); + void emitReturnReplacementNote(const Expr *LHS, StringRef LHSReplacement, + const Expr *RHS = nullptr, + StringRef RHSReplacement = StringRef(), + const BinaryOperator *ReverseBinOp = nullptr); + + unsigned MaximumLineLength; + std::unordered_set CheckedDeclStmt; +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USELESS_INTERMEDIATE_VAR_H Index: clang-tidy/readability/UselessIntermediateVarCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/readability/UselessIntermediateVarCheck.cpp @@ -0,0 +1,468 @@ +//===--- UselessIntermediateVarCheck.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 "UselessIntermediateVarCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "../utils/Matchers.h" +#include "../utils/LexerUtils.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +void UselessIntermediateVarCheck::registerMatchers(MatchFinder *Finder) { + + auto directDeclRefExprLHS1 = + // We match a direct declaration reference expression pointing + // to the variable declaration 1 as LHS. + ignoringImplicit(ignoringParenCasts(declRefExpr( + to(equalsBoundNode("varDecl1")), + expr().bind("declRefExprLHS1")))); + + 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")))); + + auto noIndirectDeclRefExpr1 = + // We match a declaration reference expression in any descendant + // pointing to variable declaration 1. + unless(hasDescendant(declRefExpr(to(equalsBoundNode("varDecl1"))))); + + 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")))); + + 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")))); + + auto noIndirectDeclRefExpr2 = + // We match a declaration reference expression in any descendant + // pointing to variable declaration 2. + unless(hasDescendant(declRefExpr(to(equalsBoundNode("varDecl2"))))); + + 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"))); + + 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"))); + + 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)))))))); + + 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 UselessIntermediateVarCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "MaximumLineLength", MaximumLineLength); +} + +void UselessIntermediateVarCheck::emitMainWarning( + const VarDecl *VarDecl1, const VarDecl *VarDecl2) { + diag(VarDecl1->getLocation(), "intermediate variable %0 is useless", + DiagnosticIDs::Warning) + << VarDecl1; + + if (VarDecl2) { + diag(VarDecl2->getLocation(), "and so is %0", + DiagnosticIDs::Warning) + << VarDecl2; + } +} + +void UselessIntermediateVarCheck::emitUsageInComparisonNote(const DeclRefExpr *VarRef, bool IsPlural) { + diag(VarRef->getLocation(), + "because %0 only used when returning the result of this comparison", + DiagnosticIDs::Note) + << (IsPlural ? "they are" : "it is"); +} + +void UselessIntermediateVarCheck::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 UselessIntermediateVarCheck::emitReturnReplacementNote( + const Expr *LHS, StringRef LHSReplacement, + const Expr *RHS, 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) { + auto ReversedBinOpText = + BinaryOperator::getOpcodeStr( + BinaryOperator::reverseComparisonOp(BinOpToReverse->getOpcode())); + + Diag << FixItHint::CreateReplacement( + SourceRange( + BinOpToReverse->getOperatorLoc(), + BinOpToReverse->getOperatorLoc().getLocWithOffset( + BinOpToReverse->getOpcodeStr().size())), + ReversedBinOpText); + + } +} + +void UselessIntermediateVarCheck::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. + 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. + auto ReturnIndentTextLength = Lexer::getIndentationForLine( + Return1->getLocStart(), Result.Context->getSourceManager()).size(); + + auto ReturnTextLength = + (*utils::lexer::getStmtText(Return1, Result.Context->getSourceManager())).size(); + + 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. + 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. + bool HasVarRef1 = VarRefLHS1 || VarRefRHS1; + bool HasVarRef2 = VarRefLHS2 || VarRefRHS2; + + // First we get the source code of the initializer expressions of the variable + // declarations. + auto Init1TextOpt = + utils::lexer::getStmtText(Init1, Result.Context->getSourceManager()); + if (!Init1TextOpt) { + return; + } + auto Init1Text = (*Init1TextOpt).str(); + + 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. + auto ReturnIndentTextLength = Lexer::getIndentationForLine( + Return2->getLocStart(), Result.Context->getSourceManager()).size(); + + 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. + 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. + 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 @@ -57,6 +57,14 @@ Improvements to clang-tidy -------------------------- +- New `readability-useless-intermediate-var + `_ check + + This new checker detects useless intermediate variables before return + statements that return the result of a simple comparison. This checker also + suggests to directly inline the initializer expression of the variable + declaration into the return expression. + - Renamed checks to use correct term "implicit conversion" instead of "implicit cast" and modified messages and option names accordingly: 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-useless-intermediate-var Index: docs/clang-tidy/checks/readability-useless-intermediate-var.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/readability-useless-intermediate-var.rst @@ -0,0 +1,19 @@ +.. title:: clang-tidy - readability-useless-intermediate-var + +readability-useless-intermediate-var +==================================== + +Detects useless 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. + +.. code-block:: c++ + + // the checker detects + + auto test = 1; + return (test == 2); + + // and suggests to fix it into + + return (1 == 2); Index: test/clang-tidy/readability-useless-intermediate-var.cpp =================================================================== --- /dev/null +++ test/clang-tidy/readability-useless-intermediate-var.cpp @@ -0,0 +1,138 @@ +// RUN: %check_clang_tidy %s readability-useless-intermediate-var %t + +bool f() { + auto test = 1; // Test + // CHECK-FIXES: {{^}} // Test{{$}} + return (test == 1); + // CHECK-FIXES: {{^}} return (1 == 1);{{$}} + // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: intermediate variable 'test' is useless [readability-useless-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration + // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here +} + +bool f2() { + auto test1 = 1; // Test1 + // CHECK-FIXES: {{^}} // Test1{{$}} + auto test2 = 2; // Test2 + // CHECK-FIXES: {{^}} // Test2{{$}} + return (test1 == test2); + // CHECK-FIXES: {{^}} return (1 == 2);{{$}} + // CHECK-MESSAGES: :[[@LINE-6]]:8: warning: intermediate variable 'test1' is useless [readability-useless-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: and so is 'test2' [readability-useless-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-4]]:11: note: because they are only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-9]]:8: note: consider removing both this variable declaration + // CHECK-MESSAGES: :[[@LINE-8]]:8: note: and this one + // CHECK-MESSAGES: :[[@LINE-7]]:11: note: and directly using the variable initialization expressions here +} + +bool f3() { + auto test1 = 1; // Test1 + // CHECK-FIXES: {{^}} // Test1{{$}} + auto test2 = 2; // Test2 + return (test1 == 2); + // CHECK-FIXES: {{^}} return (1 == 2);{{$}} + // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: intermediate variable 'test1' is useless [readability-useless-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration + // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here +} + +bool f4() { + auto test1 = 1; // Test1 + auto test2 = 2; // Test2 + // CHECK-FIXES: {{^}} // Test2{{$}} + return (test2 == 3); + // CHECK-FIXES: {{^}} return (2 == 3);{{$}} + // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: intermediate variable 'test2' is useless [readability-useless-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:11: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-6]]:8: note: consider removing the variable declaration + // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here +} + +bool f5() { + auto test1 = 1; // Test1 + // CHECK-FIXES: {{^}} // Test1{{$}} + auto test2 = 2; // Test2 + return (2 == test1); + // CHECK-FIXES: {{^}} return (1 == 2);{{$}} + // CHECK-MESSAGES: :[[@LINE-5]]:8: warning: intermediate variable 'test1' is useless [readability-useless-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-7]]:8: note: consider removing the variable declaration + // CHECK-MESSAGES: :[[@LINE-5]]:11: note: and directly using the variable initialization expression here +} + +bool f6() { + auto test1 = 1; // Test1 + auto test2 = 2; // Test2 + // CHECK-FIXES: {{^}} // Test2{{$}} + return (3 == test2); + // CHECK-FIXES: {{^}} return (2 == 3);{{$}} + // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: intermediate variable 'test2' is useless [readability-useless-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when 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: intermediate variable 'test1' is useless [readability-useless-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: intermediate variable 'test2' is useless [readability-useless-intermediate-var] + // CHECK-MESSAGES: :[[@LINE-3]]:16: note: because it is only used when returning the result of this comparison + // CHECK-MESSAGES: :[[@LINE-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 == ""); +}