From: Felix Berger Date: Wed, 14 Jul 2021 20:11:55 +0000 (-0400) Subject: [clang-tidy] performance-unnecessary-copy-initialization: Disable check when variable... X-Git-Tag: llvmorg-14-init~591 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=00edae9203c9a4f50da058d4bd25dc2e6a4930c1;p=platform%2Fupstream%2Fllvm.git [clang-tidy] performance-unnecessary-copy-initialization: Disable check when variable and initializer have different replaced template param types. This can happen when a template with two parameter types is instantiated with a single type. The fix would only be valid for this instantiation but fail for others that rely on an implicit type conversion. The test cases illustrate when the check should trigger and when not. Differential Revision: https://reviews.llvm.org/D106011 --- diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp index f6b8e44..9631e06 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp @@ -27,6 +27,8 @@ using utils::decl_ref_expr::isOnlyUsedAsConst; static constexpr StringRef ObjectArgId = "objectArg"; static constexpr StringRef InitFunctionCallId = "initFunctionCall"; +static constexpr StringRef MethodDeclId = "methodDecl"; +static constexpr StringRef FunctionDeclId = "functionDecl"; static constexpr StringRef OldVarDeclId = "oldVarDecl"; void recordFixes(const VarDecl &Var, ASTContext &Context, @@ -81,7 +83,8 @@ AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) { // returned either points to a global static variable or to a member of the // called object. return cxxMemberCallExpr( - callee(cxxMethodDecl(returns(matchers::isReferenceToConst()))), + callee(cxxMethodDecl(returns(matchers::isReferenceToConst())) + .bind(MethodDeclId)), on(declRefExpr(to(varDecl().bind(ObjectArgId))))); } @@ -89,7 +92,8 @@ AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) { // Only allow initialization of a const reference from a free function if it // has no arguments. Otherwise it could return an alias to one of its // arguments and the arguments need to be checked for const use as well. - return callExpr(callee(functionDecl(returns(matchers::isReferenceToConst()))), + return callExpr(callee(functionDecl(returns(matchers::isReferenceToConst())) + .bind(FunctionDeclId)), argumentCountIs(0), unless(callee(cxxMethodDecl()))) .bind(InitFunctionCallId); } @@ -155,6 +159,45 @@ bool isVariableUnused(const VarDecl &Var, const Stmt &BlockStmt, return allDeclRefExprs(Var, BlockStmt, Context).empty(); } +const SubstTemplateTypeParmType *getSubstitutedType(const QualType &Type, + ASTContext &Context) { + auto Matches = match( + qualType(anyOf(substTemplateTypeParmType().bind("subst"), + hasDescendant(substTemplateTypeParmType().bind("subst")))), + Type, Context); + return selectFirst("subst", Matches); +} + +bool differentReplacedTemplateParams(const QualType &VarType, + const QualType &InitializerType, + ASTContext &Context) { + if (const SubstTemplateTypeParmType *VarTmplType = + getSubstitutedType(VarType, Context)) { + if (const SubstTemplateTypeParmType *InitializerTmplType = + getSubstitutedType(InitializerType, Context)) { + return VarTmplType->getReplacedParameter() + ->desugar() + .getCanonicalType() != + InitializerTmplType->getReplacedParameter() + ->desugar() + .getCanonicalType(); + } + } + return false; +} + +QualType constructorArgumentType(const VarDecl *OldVar, + const BoundNodes &Nodes) { + if (OldVar) { + return OldVar->getType(); + } + if (const auto *FuncDecl = Nodes.getNodeAs(FunctionDeclId)) { + return FuncDecl->getReturnType(); + } + const auto *MethodDecl = Nodes.getNodeAs(MethodDeclId); + return MethodDecl->getReturnType(); +} + } // namespace UnnecessaryCopyInitialization::UnnecessaryCopyInitialization( @@ -222,6 +265,16 @@ void UnnecessaryCopyInitialization::check( if (!CtorCall->getArg(I)->isDefaultArgument()) return; + // Don't apply the check if the variable and its initializer have different + // replaced template parameter types. In this case the check triggers for a + // template instantiation where the substituted types are the same, but + // instantiations where the types differ and rely on implicit conversion would + // no longer compile if we switched to a reference. + if (differentReplacedTemplateParams( + NewVar->getType(), constructorArgumentType(OldVar, Result.Nodes), + *Result.Context)) + return; + if (OldVar == nullptr) { handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg, *Result.Context); 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 index 3ce1510..68a010c 100644 --- 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 @@ -17,6 +17,9 @@ struct ExpensiveToCopyType { Iterator end() const; void nonConstMethod(); bool constMethod() const; + template + const A &templatedAccessor() const; + operator int() const; // Implicit conversion to int. }; struct TrivialToCopyType { @@ -659,3 +662,54 @@ void negativeStructuredBinding() { C.constMethod(); D.constMethod(); } + +template +const A &templatedReference(); + +template +void negativeTemplateTypes() { + A Orig; + // Different replaced template type params do not trigger the check. In some + // template instantiation this might not be a copy but an implicit + // conversion, so converting this to a reference might not work. + B AmbiguousCopy = Orig; + // CHECK-NOT-FIXES: B AmbiguousCopy = Orig; + + B NecessaryCopy = templatedReference(); + // CHECK-NOT-FIXES: B NecessaryCopy = templatedReference(); + + B NecessaryCopy2 = Orig.template templatedAccessor(); + + // Non-dependent types in template still trigger the check. + const auto UnnecessaryCopy = ExpensiveTypeReference(); + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy' is copy-constructed + // CHECK-FIXES: const auto& UnnecessaryCopy = ExpensiveTypeReference(); + UnnecessaryCopy.constMethod(); +} + +void instantiateNegativeTemplateTypes() { + negativeTemplateTypes(); + // This template instantiation would not compile if the `AmbiguousCopy` above was made a reference. + negativeTemplateTypes(); +} + +template +void positiveSingleTemplateType() { + A Orig; + A SingleTmplParmTypeCopy = Orig; + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: local copy 'SingleTmplParmTypeCopy' of the variable 'Orig' is never modified + // CHECK-FIXES: const A& SingleTmplParmTypeCopy = Orig; + SingleTmplParmTypeCopy.constMethod(); + + A UnnecessaryCopy2 = templatedReference(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the variable 'UnnecessaryCopy2' is copy-constructed from a const reference + // CHECK-FIXES: const A& UnnecessaryCopy2 = templatedReference(); + UnnecessaryCopy2.constMethod(); + + A UnnecessaryCopy3 = Orig.template templatedAccessor(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the variable 'UnnecessaryCopy3' is copy-constructed from a const reference + // CHECK-FIXES: const A& UnnecessaryCopy3 = Orig.template templatedAccessor(); + UnnecessaryCopy3.constMethod(); +} + +void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType(); }