diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h @@ -36,6 +36,7 @@ private: void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix, const VarDecl *ObjectArg, + const CallExpr *InitCallExpr, ASTContext &Context); void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt, bool IssueFix, diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp @@ -29,6 +29,14 @@ } } +template +bool hasNonDefaultArgs(const Call &CallExpr, unsigned int StartIndex = 0) { + for (unsigned int I = StartIndex; I < CallExpr.getNumArgs(); ++I) + if (!CallExpr.getArg(I)->isDefaultArgument()) + return true; + return false; +} + } // namespace using namespace ::clang::ast_matchers; @@ -51,10 +59,12 @@ // called object. auto ConstRefReturningMethodCall = cxxMemberCallExpr(callee(cxxMethodDecl(returns(ConstReference))), - on(declRefExpr(to(varDecl().bind("objectArg"))))); + on(declRefExpr(to(varDecl().bind("objectArg"))))) + .bind("initMethodCall"); auto ConstRefReturningFunctionCall = callExpr(callee(functionDecl(returns(ConstReference))), - unless(callee(cxxMethodDecl()))); + unless(callee(cxxMethodDecl()))) + .bind("initFunctionCall"); auto localVarCopiedFrom = [this](const internal::Matcher &CopyCtorArg) { return compoundStmt( @@ -96,6 +106,10 @@ const auto *ObjectArg = Result.Nodes.getNodeAs("objectArg"); const auto *BlockStmt = Result.Nodes.getNodeAs("blockStmt"); const auto *CtorCall = Result.Nodes.getNodeAs("ctorCall"); + const auto *InitFunctionCall = + Result.Nodes.getNodeAs("initFunctionCall"); + const auto *InitMethodCall = + Result.Nodes.getNodeAs("initMethodCall"); TraversalKindScope RAII(*Result.Context, ast_type_traits::TK_AsIs); @@ -108,12 +122,13 @@ // 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 (hasNonDefaultArgs(*CtorCall, 1)) + return; if (OldVar == nullptr) { handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg, + InitMethodCall != nullptr ? InitMethodCall + : InitFunctionCall, *Result.Context); } else { handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix, @@ -123,14 +138,15 @@ void UnnecessaryCopyInitialization::handleCopyFromMethodReturn( const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix, - const VarDecl *ObjectArg, ASTContext &Context) { + const VarDecl *ObjectArg, const CallExpr *InitCallExpr, + ASTContext &Context) { bool IsConstQualified = Var.getType().isConstQualified(); - if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context)) + if (hasNonDefaultArgs(*InitCallExpr) || + (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))) return; if (ObjectArg != nullptr && !isOnlyUsedAsConst(*ObjectArg, BlockStmt, Context)) return; - auto Diagnostic = diag(Var.getLocation(), IsConstQualified ? "the const qualified variable %0 is " diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp @@ -6,6 +6,8 @@ const ExpensiveToCopyType &reference() const; void nonConstMethod(); bool constMethod() const; + const ExpensiveToCopyType &methodWithArg(const ExpensiveToCopyType &) const; + const ExpensiveToCopyType &methodWithDefaultArg(int arg = 0) const; }; struct TrivialToCopyType { @@ -23,6 +25,9 @@ ExpensiveToCopyType global_expensive_to_copy_type; const ExpensiveToCopyType &ExpensiveTypeReference(); +const ExpensiveToCopyType &freeFunctionWithArg(const ExpensiveToCopyType &); +const ExpensiveToCopyType &freeFunctionWithDefaultArg( + const ExpensiveToCopyType *arg = nullptr); const TrivialToCopyType &TrivialTypeReference(); void mutate(ExpensiveToCopyType &); @@ -387,3 +392,38 @@ for (const Element &E : Container()) { } } + +// This should not trigger the check as the argument could introduce an alias. +void negativeInitializedFromMethodWithArg() { + ExpensiveToCopyType Orig; + const ExpensiveToCopyType Copy = Orig.methodWithArg(Orig); +} + +// This should not trigger the check as the argument could introduce an alias. +void negativeInitializedFromFreeFunctionWithArg() { + ExpensiveToCopyType Orig; + const ExpensiveToCopyType Copy = freeFunctionWithArg(Orig); +} + +void positiveInitializedFromMethodWithDefaultArg() { + ExpensiveToCopyType Orig; + const ExpensiveToCopyType Copy = Orig.methodWithDefaultArg(); + // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'Copy' + // CHECK-FIXES: const ExpensiveToCopyType& Copy = Orig.methodWithDefaultArg(); +} + +void negativeInitializedFromMethodWithNonDefaultArg() { + ExpensiveToCopyType Orig; + const ExpensiveToCopyType Copy = Orig.methodWithDefaultArg(5); +} + +void positiveInitializedFromFreeFunctionWithDefaultArg() { + const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg(); + // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'Copy' + // CHECK-FIXES: const ExpensiveToCopyType& Copy = freeFunctionWithDefaultArg(); +} + +void negativeInitialzedFromFreeFunctionWithNonDefaultArg() { + ExpensiveToCopyType Orig; + const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg(&Orig); +}