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,19 @@ /// 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(clang::CompilerInstance &Compiler) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + void handleMoveFix(const ParmVarDecl &Var, + const DeclRefExpr &CopyArgument, + const SourceManager &SM); + + 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,9 @@ #include "../utils/DeclRefExprUtils.h" #include "../utils/FixItHintUtils.h" #include "../utils/Matchers.h" +#include "../utils/TypeTraits.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Preprocessor.h" using namespace clang::ast_matchers; @@ -29,6 +32,12 @@ } // 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(), @@ -64,6 +73,28 @@ !utils::decl_ref_expr::isOnlyUsedAsConst( *Param, *Function->getBody(), *Result.Context))) 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 Matches = utils::decl_ref_expr::allDeclRefExprs( + *Param, *Function->getBody(), *Result.Context); + auto CanonicalType = Param->getType().getCanonicalType(); + if (Matches.size() == 1 && + ((utils::type_traits::hasMoveConstructor(CanonicalType) && + utils::decl_ref_expr::isCopyConstructorArgument( + **Matches.begin(), *Function->getBody(), *Result.Context)) || + (utils::type_traits::hasMoveAssignmentOperator(CanonicalType) && + utils::decl_ref_expr::isCopyAssignmentArgument( + **Matches.begin(), *Function->getBody(), *Result.Context)))) { + handleMoveFix(*Param, **Matches.begin(), *Result.SourceManager); + return; + } + } + auto Diag = diag(Param->getLocation(), IsConstQualified ? "the const qualified parameter %0 is " @@ -86,6 +117,38 @@ } } +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 SourceManager &SM) { + 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; + Diag << FixItHint::CreateReplacement( + CopyArgument.getSourceRange(), + (Twine("std::move(") + Var.getName() + ")").str()); + 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,19 @@ 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 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,17 +38,6 @@ 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; -} - // 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. SmallPtrSet @@ -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(findAll(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(findAll(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/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,28 @@ return false; } +bool hasMoveConstructor(QualType Type) { + auto *Record = Type->getAsCXXRecordDecl(); + if (!Record || !Record->hasDefinition()) + return false; + for (const auto *Constructor : Record->ctors()) { + if (Constructor->isMoveConstructor() && !Constructor->isDeleted()) + return true; + } + return false; +} + +bool hasMoveAssignmentOperator(QualType Type) { + auto *Record = Type->getAsCXXRecordDecl(); + if (!Record || !Record->hasDefinition()) + return false; + for (const auto *Method : Record->methods()) { + if (Method->isMoveAssignmentOperator() && !Method->isDeleted()) + return true; + } + return false; +} + } // 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 &&) = default; + ExpensiveMovableType(const ExpensiveMovableType &) = default; + ExpensiveMovableType &operator=(const ExpensiveMovableType &) = default; + ExpensiveMovableType &operator=(ExpensiveMovableType &&) = default; + ~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; +}