Index: clang-tidy/performance/UnnecessaryCopyInitialization.h =================================================================== --- clang-tidy/performance/UnnecessaryCopyInitialization.h +++ clang-tidy/performance/UnnecessaryCopyInitialization.h @@ -26,10 +26,16 @@ // const references, and const pointers to const. class UnnecessaryCopyInitialization : public ClangTidyCheck { public: - UnnecessaryCopyInitialization(StringRef Name, ClangTidyContext *Context) + UnnecessaryCopyInitialization(StringRef Name, ClangTidyContext* Context) : ClangTidyCheck(Name, Context) {} - void registerMatchers(ast_matchers::MatchFinder *Finder) override; - void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void registerMatchers(ast_matchers::MatchFinder* Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult& Result) override; + +private: + void handleCopyFromMethodReturn(const VarDecl& Var, const Stmt& BlockStmt, + ASTContext& Context); + void handleCopyFromLocalVar(const VarDecl& NewVar, const VarDecl& OldVar, + const Stmt& BlockStmt, ASTContext& Context); }; } // namespace performance Index: clang-tidy/performance/UnnecessaryCopyInitialization.cpp =================================================================== --- clang-tidy/performance/UnnecessaryCopyInitialization.cpp +++ clang-tidy/performance/UnnecessaryCopyInitialization.cpp @@ -18,14 +18,15 @@ namespace performance { using namespace ::clang::ast_matchers; +using clang::tidy::decl_ref_expr_utils::isOnlyUsedAsConst; -void UnnecessaryCopyInitialization::registerMatchers( - ast_matchers::MatchFinder *Finder) { +void UnnecessaryCopyInitialization::registerMatchers(MatchFinder* Finder) { auto ConstReference = referenceType(pointee(qualType(isConstQualified()))); auto ConstOrConstReference = allOf(anyOf(ConstReference, isConstQualified()), unless(allOf(pointerType(), unless(pointerType(pointee( qualType(isConstQualified()))))))); + // Match method call expressions where the this argument is a const // type or const reference. This returned const reference is highly likely to // outlive the local const reference of the variable being declared. @@ -37,45 +38,96 @@ auto ConstRefReturningFunctionCall = callExpr(callee(functionDecl(returns(ConstReference))), unless(callee(cxxMethodDecl()))); + + auto localVarCopiedFrom = []( + const ast_matchers::internal::Matcher& CopyCtorArg) { + return compoundStmt( + forEachDescendant( + varDecl(hasLocalStorage(), + hasType(matchers::isExpensiveToCopy()), + hasInitializer(cxxConstructExpr( + hasDeclaration(cxxConstructorDecl( + isCopyConstructor())), + hasArgument(0, CopyCtorArg)) + .bind("ctorCall"))) + .bind("newVarDecl"))) + .bind("blockStmt"); + }; + Finder->addMatcher( - compoundStmt( - forEachDescendant( - varDecl( - hasLocalStorage(), hasType(matchers::isExpensiveToCopy()), - hasInitializer(cxxConstructExpr( - hasDeclaration(cxxConstructorDecl(isCopyConstructor())), - hasArgument( - 0, anyOf(ConstRefReturningFunctionCall, - ConstRefReturningMethodCallOfConstParam))))) - .bind("varDecl"))).bind("blockStmt"), + localVarCopiedFrom(anyOf(ConstRefReturningFunctionCall, + ConstRefReturningMethodCallOfConstParam)), this); + + Finder->addMatcher(localVarCopiedFrom(declRefExpr( + to(varDecl(hasLocalStorage()).bind("oldVarDecl")))), + this); } void UnnecessaryCopyInitialization::check( - const ast_matchers::MatchFinder::MatchResult &Result) { - const auto *Var = Result.Nodes.getNodeAs("varDecl"); - const auto *BlockStmt = Result.Nodes.getNodeAs("blockStmt"); - bool IsConstQualified = Var->getType().isConstQualified(); - if (!IsConstQualified && - !decl_ref_expr_utils::isOnlyUsedAsConst(*Var, *BlockStmt, - *Result.Context)) + const MatchFinder::MatchResult& Result) { + const auto* NewVar = Result.Nodes.getNodeAs("newVarDecl"); + const auto* OldVar = Result.Nodes.getNodeAs("oldVarDecl"); + const auto* BlockStmt = Result.Nodes.getNodeAs("blockStmt"); + const auto* CtorCall = Result.Nodes.getNodeAs("ctorCall"); + + // A constructor that looks like T(const T& t, bool arg = false) counts as a + // copy only when it is called with default arguments for the arguments after + // the first. + for (unsigned int i = 1; i < CtorCall->getNumArgs(); ++i) + if (!CtorCall->getArg(i)->isDefaultArgument()) + return; + + if (OldVar == nullptr) { + handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Result.Context); + } else { + handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Result.Context); + } +} + +namespace { +void recordFixes(const VarDecl& Var, ASTContext& Context, + DiagnosticBuilder& Diagnostic) { + // Do not propose fixes in macros since we cannot place them correctly. + if (Var.getLocation().isMacroID()) return; - DiagnosticBuilder Diagnostic = - diag(Var->getLocation(), - IsConstQualified ? "the const qualified variable '%0' is " + + Diagnostic << utils::create_fix_it::changeVarDeclToReference(Var, Context); + if (!Var.getType().isLocalConstQualified()) + Diagnostic << utils::create_fix_it::changeVarDeclToConst(Var); +} +} // namespace + +void UnnecessaryCopyInitialization::handleCopyFromMethodReturn( + const VarDecl& Var, const Stmt& BlockStmt, ASTContext& Context) { + bool IsConstQualified = Var.getType().isConstQualified(); + if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context)) + return; + + auto Diagnostic = + diag(Var.getLocation(), + IsConstQualified ? "the const qualified variable %0 is " "copy-constructed from a const reference; " "consider making it a const reference" - : "the variable '%0' is copy-constructed from a " + : "the variable %0 is copy-constructed from a " "const reference but is only used as const " "reference; consider making it a const reference") - << Var->getName(); - // Do not propose fixes in macros since we cannot place them correctly. - if (Var->getLocStart().isMacroID()) + << &Var; + recordFixes(Var, Context, Diagnostic); +} + +void UnnecessaryCopyInitialization::handleCopyFromLocalVar( + const VarDecl& NewVar, const VarDecl& OldVar, const Stmt& BlockStmt, + ASTContext& Context) { + if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) || + !isOnlyUsedAsConst(OldVar, BlockStmt, Context)) return; - Diagnostic << utils::create_fix_it::changeVarDeclToReference(*Var, - *Result.Context); - if (!IsConstQualified) - Diagnostic << utils::create_fix_it::changeVarDeclToConst(*Var); + + auto Diagnostic = diag(NewVar.getLocation(), + "local copy %0 of the variable %1 is never modified, " + "consider avoiding the copy") + << &NewVar << &OldVar; + recordFixes(NewVar, Context, Diagnostic); } } // namespace performance Index: docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst =================================================================== --- docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst +++ docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst @@ -17,18 +17,21 @@ .. code-block:: c++ - const string& constReference() { - } - void function() { + const string& constReference(); + void Function() { // The warning will suggest making this a const reference. const string UnnecessaryCopy = constReference(); } struct Foo { const string& name() const; - } + }; void Function(const Foo& foo) { // The warning will suggest making this a const reference. - string UnnecessaryCopy = foo.name(); - UnnecessaryCopy.find("bar"); + string UnnecessaryCopy1 = foo.name(); + UnnecessaryCopy1.find("bar"); + + // The warning will suggest making this a const reference. + string UnnecessaryCopy2 = UnnecessaryCopy1; + UnnecessaryCopy2.find("bar"); } Index: test/clang-tidy/performance-unnecessary-copy-initialization.cpp =================================================================== --- test/clang-tidy/performance-unnecessary-copy-initialization.cpp +++ test/clang-tidy/performance-unnecessary-copy-initialization.cpp @@ -1,28 +1,33 @@ // RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t struct ExpensiveToCopyType { - ExpensiveToCopyType() {} - virtual ~ExpensiveToCopyType() {} - const ExpensiveToCopyType &reference() const { return *this; } - void nonConstMethod() {} + ExpensiveToCopyType(); + virtual ~ExpensiveToCopyType(); + const ExpensiveToCopyType &reference() const; + void nonConstMethod(); + bool constMethod() const; }; struct TrivialToCopyType { - const TrivialToCopyType &reference() const { return *this; } + const TrivialToCopyType &reference() const; }; -const ExpensiveToCopyType &ExpensiveTypeReference() { - static const ExpensiveToCopyType *Type = new ExpensiveToCopyType(); - return *Type; -} +struct WeirdCopyCtorType { + WeirdCopyCtorType(); + WeirdCopyCtorType(const WeirdCopyCtorType& w, bool oh_yes = true); -const TrivialToCopyType &TrivialTypeReference() { - static const TrivialToCopyType *Type = new TrivialToCopyType(); - return *Type; -} + void nonConstMethod(); + bool constMethod() const; +}; + +ExpensiveToCopyType global_expensive_to_copy_type; + +const ExpensiveToCopyType &ExpensiveTypeReference(); +const TrivialToCopyType &TrivialTypeReference(); void mutate(ExpensiveToCopyType &); void mutate(ExpensiveToCopyType *); +void useAsConstPointer(const ExpensiveToCopyType*); void useAsConstReference(const ExpensiveToCopyType &); void useByValue(ExpensiveToCopyType); @@ -223,3 +228,101 @@ // Ensure fix is not applied. // CHECK-FIXES: auto CopyInMacroArg = Obj.reference() } + +void PositiveLocalCopyConstMethodInvoked() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_1 = orig; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_1' of the variable 'orig' is never modified, consider avoiding the copy [performance-unnecessary-copy-initialization] + // CHECK-FIXES: const ExpensiveToCopyType& copy_1 = orig; + copy_1.constMethod(); + orig.constMethod(); +} + +void PositiveLocalCopyUsingExplicitCopyCtor() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_2(orig); + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_2' + // CHECK-FIXES: const ExpensiveToCopyType& copy_2(orig); + copy_2.constMethod(); + orig.constMethod(); +} + +void PositiveLocalCopyCopyIsArgument(const ExpensiveToCopyType& orig) { + ExpensiveToCopyType copy_3 = orig; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_3' + // CHECK-FIXES: const ExpensiveToCopyType& copy_3 = orig; + copy_3.constMethod(); +} + +void PositiveLocalCopyUsedAsConstRef() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_4 = orig; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_4' + // CHECK-FIXES: const ExpensiveToCopyType& copy_4 = orig; + useAsConstReference(orig); +} + +void PositiveLocalCopyWeirdCopy() { + WeirdCopyCtorType orig; + WeirdCopyCtorType weird_1(orig); + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: local copy 'weird_1' + // CHECK-FIXES: const WeirdCopyCtorType& weird_1(orig); + weird_1.constMethod(); + + WeirdCopyCtorType weird_2 = orig; + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: local copy 'weird_2' + // CHECK-FIXES: const WeirdCopyCtorType& weird_2 = orig; + weird_2.constMethod(); +} + +void NegativeLocalCopySimpleTypes() { + int i1 = 0; + int i2 = i1; +} + +void NegativeLocalCopyCopyIsModified() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_5 = orig; + copy_5.nonConstMethod(); +} + +void NegativeLocalCopyOriginalIsModified() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_6 = orig; + orig.nonConstMethod(); +} + +void NegativeLocalCopyUsedAsRefArg() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_7 = orig; + mutate(copy_7); +} + +void NegativeLocalCopyUsedAsPointerArg() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_8 = orig; + mutate(©_8); +} + +void NegativeLocalCopyCopyFromGlobal() { + ExpensiveToCopyType copy_9 = global_expensive_to_copy_type; +} + +void NegativeLocalCopyCopyToStatic() { + ExpensiveToCopyType orig; + static ExpensiveToCopyType copy_10 = orig; +} + +void NegativeLocalCopyNonConstInForLoop() { + ExpensiveToCopyType orig; + for (ExpensiveToCopyType copy_11 = orig; orig.constMethod(); + orig.nonConstMethod()) { + orig.constMethod(); + } +} + +void NegativeLocalCopyWeirdNonCopy() { + WeirdCopyCtorType orig; + WeirdCopyCtorType weird_3(orig, false); + WeirdCopyCtorType weird_4(orig, true); +}