Index: clang-tidy/performance/UnnecessaryValueParamCheck.h =================================================================== --- clang-tidy/performance/UnnecessaryValueParamCheck.h +++ clang-tidy/performance/UnnecessaryValueParamCheck.h @@ -11,6 +11,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_VALUE_PARAM_H #include "../ClangTidy.h" +#include "../utils/IncludeInserter.h" namespace clang { namespace tidy { @@ -23,10 +24,18 @@ /// http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html class UnnecessaryValueParamCheck : public ClangTidyCheck { public: - UnnecessaryValueParamCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + UnnecessaryValueParamCheck(StringRef Name, ClangTidyContext *Context); void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void registerPPCallbacks(CompilerInstance &Compiler) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + void handleMoveFix(const ParmVarDecl &Var, const DeclRefExpr &CopyArgument, + const ASTContext &Context); + + std::unique_ptr Inserter; + const utils::IncludeSorter::IncludeStyle IncludeStyle; }; } // namespace performance Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp =================================================================== --- clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp @@ -12,6 +12,10 @@ #include "../utils/DeclRefExprUtils.h" #include "../utils/FixItHintUtils.h" #include "../utils/Matchers.h" +#include "../utils/TypeTraits.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Lexer.h" +#include "clang/Lex/Preprocessor.h" using namespace clang::ast_matchers; @@ -27,8 +31,21 @@ .str(); } +template bool isSetDifferenceEmpty(const S &S1, const S &S2) { + for (const auto &E : S1) + if (S2.count(E) == 0) + return false; + return true; +} + } // namespace +UnnecessaryValueParamCheck::UnnecessaryValueParamCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IncludeStyle(utils::IncludeSorter::parseIncludeStyle( + Options.get("IncludeStyle", "llvm"))) {} + void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) { const auto ExpensiveValueParamDecl = parmVarDecl(hasType(hasCanonicalType(allOf(matchers::isExpensiveToCopy(), @@ -58,12 +75,39 @@ // Do not trigger on non-const value parameters when: // 1. they are in a constructor definition since they can likely trigger // misc-move-constructor-init which will suggest to move the argument. - // 2. they are not only used as const. if (!IsConstQualified && (llvm::isa(Function) || - !Function->doesThisDeclarationHaveABody() || - !utils::decl_ref_expr::isOnlyUsedAsConst( - *Param, *Function->getBody(), *Result.Context))) + !Function->doesThisDeclarationHaveABody())) return; + + auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs( + *Param, *Function->getBody(), *Result.Context); + auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs( + *Param, *Function->getBody(), *Result.Context); + // 2. they are not only used as const. + if (!isSetDifferenceEmpty(AllDeclRefExprs, ConstDeclRefExprs)) + return; + + // If the parameter is non-const, check if it has a move constructor and is + // only referenced once to copy-construct another object or whether it has a + // move assignment operator and is only referenced once when copy-assigned. + // In this case wrap DeclRefExpr with std::move() to avoid the unnecessary + // copy. + if (!IsConstQualified) { + auto CanonicalType = Param->getType().getCanonicalType(); + if (AllDeclRefExprs.size() == 1 && + ((utils::type_traits::hasMoveConstructor(CanonicalType) && + utils::decl_ref_expr::isCopyConstructorArgument( + **AllDeclRefExprs.begin(), *Function->getBody(), + *Result.Context)) || + (utils::type_traits::hasMoveAssignmentOperator(CanonicalType) && + utils::decl_ref_expr::isCopyAssignmentArgument( + **AllDeclRefExprs.begin(), *Function->getBody(), + *Result.Context)))) { + handleMoveFix(*Param, **AllDeclRefExprs.begin(), *Result.Context); + return; + } + } + auto Diag = diag(Param->getLocation(), IsConstQualified ? "the const qualified parameter %0 is " @@ -86,6 +130,40 @@ } } +void UnnecessaryValueParamCheck::registerPPCallbacks( + CompilerInstance &Compiler) { + Inserter.reset(new utils::IncludeInserter( + Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle)); + Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks()); +} + +void UnnecessaryValueParamCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IncludeStyle", + utils::IncludeSorter::toString(IncludeStyle)); +} + +void UnnecessaryValueParamCheck::handleMoveFix(const ParmVarDecl &Var, + const DeclRefExpr &CopyArgument, + const ASTContext &Context) { + auto Diag = diag(CopyArgument.getLocStart(), + "parameter %0 is passed by value and only copied once; " + "consider moving it to avoid unnecessary copies") + << &Var; + // Do not propose fixes in macros since we cannot place them correctly. + if (CopyArgument.getLocStart().isMacroID()) + return; + const auto &SM = Context.getSourceManager(); + auto EndLoc = Lexer::getLocForEndOfToken(CopyArgument.getLocation(), 0, SM, + Context.getLangOpts()); + Diag << FixItHint::CreateInsertion(CopyArgument.getLocStart(), "std::move(") + << FixItHint::CreateInsertion(EndLoc, ")"); + if (auto IncludeFixit = Inserter->CreateIncludeInsertion( + SM.getFileID(CopyArgument.getLocStart()), "utility", + /*IsAngled=*/true)) + Diag << *IncludeFixit; +} + } // namespace performance } // namespace tidy } // namespace clang Index: clang-tidy/utils/CMakeLists.txt =================================================================== --- clang-tidy/utils/CMakeLists.txt +++ clang-tidy/utils/CMakeLists.txt @@ -8,6 +8,7 @@ IncludeInserter.cpp IncludeSorter.cpp LexerUtils.cpp + Matchers.cpp OptionsUtils.cpp TypeTraits.cpp Index: clang-tidy/utils/DeclRefExprUtils.h =================================================================== --- clang-tidy/utils/DeclRefExprUtils.h +++ clang-tidy/utils/DeclRefExprUtils.h @@ -12,6 +12,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Type.h" +#include "llvm/ADT/SmallPtrSet.h" namespace clang { namespace tidy { @@ -26,6 +27,25 @@ bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt, ASTContext &Context); +// Returns set of all DeclRefExprs to VarDecl in Stmt. +llvm::SmallPtrSet +allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context); + +// Returns set of all DeclRefExprs where VarDecl is guaranteed to be accessed in +// a const fashion. +llvm::SmallPtrSet +constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, + ASTContext &Context); + +// Returns true if DeclRefExpr is the argument of a copy-constructor call expr. +bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt, + ASTContext &Context); + +// Returns true if DeclRefExpr is the argument of a copy-assignment operator +// call expr. +bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt, + ASTContext &Context); + } // namespace decl_ref_expr } // namespace utils } // namespace tidy Index: clang-tidy/utils/DeclRefExprUtils.cpp =================================================================== --- clang-tidy/utils/DeclRefExprUtils.cpp +++ clang-tidy/utils/DeclRefExprUtils.cpp @@ -8,10 +8,10 @@ //===----------------------------------------------------------------------===// #include "DeclRefExprUtils.h" +#include "Matchers.h" #include "clang/AST/ASTContext.h" #include "clang/AST/DeclCXX.h" #include "clang/ASTMatchers/ASTMatchFinder.h" -#include "llvm/ADT/SmallPtrSet.h" namespace clang { namespace tidy { @@ -38,16 +38,7 @@ Nodes.insert(Match.getNodeAs(ID)); } -// Finds all DeclRefExprs to VarDecl in Stmt. -SmallPtrSet -declRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) { - auto Matches = match( - findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")), - Stmt, Context); - SmallPtrSet DeclRefs; - extractNodesByIdTo(Matches, "declRef", DeclRefs); - return DeclRefs; -} +} // namespace // Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl // is the a const reference or value argument to a CallExpr or CXXConstructExpr. @@ -80,8 +71,6 @@ return DeclRefs; } -} // namespace - bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt, ASTContext &Context) { // Collect all DeclRefExprs to the loop variable and all CallExprs and @@ -89,11 +78,48 @@ // reference parameter. // If the difference is empty it is safe for the loop variable to be a const // reference. - auto AllDeclRefs = declRefExprs(Var, Stmt, Context); + auto AllDeclRefs = allDeclRefExprs(Var, Stmt, Context); auto ConstReferenceDeclRefs = constReferenceDeclRefExprs(Var, Stmt, Context); return isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs); } +SmallPtrSet +allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) { + auto Matches = match( + findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")), + Stmt, Context); + SmallPtrSet DeclRefs; + extractNodesByIdTo(Matches, "declRef", DeclRefs); + return DeclRefs; +} + +bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt, + ASTContext &Context) { + auto UsedAsConstRefArg = forEachArgumentWithParam( + declRefExpr(equalsNode(&DeclRef)), + parmVarDecl(hasType(matchers::isConstReference()))); + auto Matches = match( + stmt(hasDescendant( + cxxConstructExpr(UsedAsConstRefArg, hasDeclaration(cxxConstructorDecl( + isCopyConstructor()))) + .bind("constructExpr"))), + Stmt, Context); + return !Matches.empty(); +} + +bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt, + ASTContext &Context) { + auto UsedAsConstRefArg = forEachArgumentWithParam( + declRefExpr(equalsNode(&DeclRef)), + parmVarDecl(hasType(matchers::isConstReference()))); + auto Matches = match( + stmt(hasDescendant( + cxxOperatorCallExpr(UsedAsConstRefArg, hasOverloadedOperatorName("=")) + .bind("operatorCallExpr"))), + Stmt, Context); + return !Matches.empty(); +} + } // namespace decl_ref_expr } // namespace utils } // namespace tidy Index: clang-tidy/utils/Matchers.h =================================================================== --- clang-tidy/utils/Matchers.h +++ clang-tidy/utils/Matchers.h @@ -40,6 +40,9 @@ Node, Finder->getASTContext()); } +// Returns QualType matcher for references to const. +ast_matchers::TypeMatcher isConstReference(); + } // namespace matchers } // namespace tidy } // namespace clang Index: clang-tidy/utils/Matchers.cpp =================================================================== --- /dev/null +++ clang-tidy/utils/Matchers.cpp @@ -0,0 +1,23 @@ +//===--- Matchers.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 "Matchers.h" + +namespace clang { +namespace tidy { +namespace matchers { + +ast_matchers::TypeMatcher isConstReference() { + return ast_matchers::referenceType(ast_matchers::pointee( + ast_matchers::qualType(ast_matchers::isConstQualified()))); +} + +} // namespace matchers +} // namespace tidy +} // namespace clang Index: clang-tidy/utils/TypeTraits.h =================================================================== --- clang-tidy/utils/TypeTraits.h +++ clang-tidy/utils/TypeTraits.h @@ -29,6 +29,12 @@ bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl, const ASTContext &Context); +// Returns true if Type has a non-deleted move constructor. +bool hasMoveConstructor(QualType Type); + +// Return true if Type has a non-deleted move assignment operator. +bool hasMoveAssignmentOperator(QualType Type); + } // type_traits } // namespace utils } // namespace tidy Index: clang-tidy/utils/TypeTraits.cpp =================================================================== --- clang-tidy/utils/TypeTraits.cpp +++ clang-tidy/utils/TypeTraits.cpp @@ -123,6 +123,20 @@ return false; } +bool hasMoveConstructor(QualType Type) { + auto *Record = Type->getAsCXXRecordDecl(); + if (!Record || !Record->hasDefinition()) + return false; + return Record->hasNonTrivialMoveConstructor(); +} + +bool hasMoveAssignmentOperator(QualType Type) { + auto *Record = Type->getAsCXXRecordDecl(); + if (!Record || !Record->hasDefinition()) + return false; + return Record->hasNonTrivialMoveAssignment(); +} + } // namespace type_traits } // namespace utils } // namespace tidy Index: test/clang-tidy/performance-unnecessary-value-param.cpp =================================================================== --- test/clang-tidy/performance-unnecessary-value-param.cpp +++ test/clang-tidy/performance-unnecessary-value-param.cpp @@ -1,5 +1,7 @@ // RUN: %check_clang_tidy %s performance-unnecessary-value-param %t +// CHECK-FIXES: #include + struct ExpensiveToCopyType { const ExpensiveToCopyType & constReference() const { return *this; @@ -30,6 +32,15 @@ void constMethod() const; }; +struct ExpensiveMovableType { + ExpensiveMovableType(); + ExpensiveMovableType(ExpensiveMovableType &&); + ExpensiveMovableType(const ExpensiveMovableType &) = default; + ExpensiveMovableType &operator=(const ExpensiveMovableType &) = default; + ExpensiveMovableType &operator=(ExpensiveMovableType &&); + ~ExpensiveMovableType(); +}; + void positiveExpensiveConstValue(const ExpensiveToCopyType Obj); // CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj); void positiveExpensiveConstValue(const ExpensiveToCopyType Obj) { @@ -180,3 +191,36 @@ void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) { M.constMethod(); } + +void PositiveMoveOnCopyConstruction(ExpensiveMovableType E) { + auto F = E; + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: parameter 'E' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param] + // CHECK-FIXES: auto F = std::move(E); +} + +void PositiveConstRefNotMoveSinceReferencedMultipleTimes(ExpensiveMovableType E) { + // CHECK-MESSAGES: [[@LINE-1]]:79: warning: the parameter 'E' is copied + // CHECK-FIXES: void PositiveConstRefNotMoveSinceReferencedMultipleTimes(const ExpensiveMovableType& E) { + auto F = E; + auto G = E; +} + +void PositiveMoveOnCopyAssignment(ExpensiveMovableType E) { + ExpensiveMovableType F; + F = E; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: parameter 'E' is passed by value + // CHECK-FIXES: F = std::move(E); +} + +void PositiveConstRefNotMoveConstructible(ExpensiveToCopyType T) { + // CHECK-MESSAGES: [[@LINE-1]]:63: warning: the parameter 'T' is copied + // CHECK-FIXES: void PositiveConstRefNotMoveConstructible(const ExpensiveToCopyType& T) { + auto U = T; +} + +void PositiveConstRefNotMoveAssignable(ExpensiveToCopyType A) { + // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the parameter 'A' is copied + // CHECK-FIXES: void PositiveConstRefNotMoveAssignable(const ExpensiveToCopyType& A) { + ExpensiveToCopyType B; + B = A; +}