Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h +++ clang-tools-extra/trunk/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-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ clang-tools-extra/trunk/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,22 @@ .str(); } +template +bool isSubset(const S &SubsetCandidate, const S &SupersetCandidate) { + for (const auto &E : SubsetCandidate) + if (SupersetCandidate.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 +76,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 (!isSubset(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::hasNonTrivialMoveConstructor(CanonicalType) && + utils::decl_ref_expr::isCopyConstructorArgument( + **AllDeclRefExprs.begin(), *Function->getBody(), + *Result.Context)) || + (utils::type_traits::hasNonTrivialMoveAssignment(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 +131,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-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.h +++ clang-tools-extra/trunk/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 { @@ -27,6 +28,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-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp +++ clang-tools-extra/trunk/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::isReferenceToConst()))); + 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::isReferenceToConst()))); + 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-tools-extra/trunk/clang-tidy/utils/Matchers.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/Matchers.h +++ clang-tools-extra/trunk/clang-tidy/utils/Matchers.h @@ -42,6 +42,12 @@ AST_MATCHER(FieldDecl, isBitfield) { return Node.isBitField(); } +// Returns QualType matcher for references to const. +AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) { + using namespace ast_matchers; + return referenceType(pointee(qualType(isConstQualified()))); +} + } // namespace matchers } // namespace tidy } // namespace clang Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h +++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h @@ -29,6 +29,12 @@ bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl, const ASTContext &Context); +/// Returns true if `Type` has a non-trivial move constructor. +bool hasNonTrivialMoveConstructor(QualType Type); + +/// Return true if `Type` has a non-trivial move assignment operator. +bool hasNonTrivialMoveAssignment(QualType Type); + } // type_traits } // namespace utils } // namespace tidy Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp +++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp @@ -124,6 +124,18 @@ return false; } +bool hasNonTrivialMoveConstructor(QualType Type) { + auto *Record = Type->getAsCXXRecordDecl(); + return Record && Record->hasDefinition() && + Record->hasNonTrivialMoveConstructor(); +} + +bool hasNonTrivialMoveAssignment(QualType Type) { + auto *Record = Type->getAsCXXRecordDecl(); + return Record && Record->hasDefinition() && + Record->hasNonTrivialMoveAssignment(); +} + } // namespace type_traits } // namespace utils } // namespace tidy Index: clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst @@ -10,7 +10,7 @@ which means they are not trivially copyable or have a non-trivial copy constructor or destructor. -To ensure that it is safe to replace the value paramater with a const reference +To ensure that it is safe to replace the value parameter with a const reference the following heuristic is employed: 1. the parameter is const qualified; @@ -31,3 +31,25 @@ Value.ConstMethd(); ExpensiveToCopy Copy(Value); } + +If the parameter is not const, only copied or assigned once and has a +non-trivial move-constructor or move-assignment operator respectively the check +will suggest to move it. + +Example: + +.. code-block:: c++ + + void setValue(string Value) { + Field = Value; + } + +Will become: + +.. code-block:: c++ + + #include + + void setValue(string Value) { + Field = std::move(Value); + } Index: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp +++ clang-tools-extra/trunk/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; +}