[clang-tools-extra] 00edae9 - [clang-tidy] performance-unnecessary-copy-initialization: Disable check when variable and initializer have different replaced template param types.
Felix Berger via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 22 12:17:59 PDT 2021
Author: Felix Berger
Date: 2021-07-22T15:17:24-04:00
New Revision: 00edae9203c9a4f50da058d4bd25dc2e6a4930c1
URL: https://github.com/llvm/llvm-project/commit/00edae9203c9a4f50da058d4bd25dc2e6a4930c1
DIFF: https://github.com/llvm/llvm-project/commit/00edae9203c9a4f50da058d4bd25dc2e6a4930c1.diff
LOG: [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
Added:
Modified:
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index f6b8e44785b5f..9631e06a8f95b 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<SubstTemplateTypeParmType>("subst", Matches);
+}
+
+bool
diff erentReplacedTemplateParams(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<FunctionDecl>(FunctionDeclId)) {
+ return FuncDecl->getReturnType();
+ }
+ const auto *MethodDecl = Nodes.getNodeAs<CXXMethodDecl>(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
diff erent
+ // 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
diff er and rely on implicit conversion would
+ // no longer compile if we switched to a reference.
+ if (
diff erentReplacedTemplateParams(
+ 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 3ce151035d5be..68a010c4d953c 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<ExpensiveToCopyType> end() const;
void nonConstMethod();
bool constMethod() const;
+ template <typename A>
+ const A &templatedAccessor() const;
+ operator int() const; // Implicit conversion to int.
};
struct TrivialToCopyType {
@@ -659,3 +662,54 @@ void negativeStructuredBinding() {
C.constMethod();
D.constMethod();
}
+
+template <typename A>
+const A &templatedReference();
+
+template <typename A, typename B>
+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<A>();
+ // CHECK-NOT-FIXES: B NecessaryCopy = templatedReference<A>();
+
+ B NecessaryCopy2 = Orig.template templatedAccessor<A>();
+
+ // 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<ExpensiveToCopyType, ExpensiveToCopyType>();
+ // This template instantiation would not compile if the `AmbiguousCopy` above was made a reference.
+ negativeTemplateTypes<ExpensiveToCopyType, int>();
+}
+
+template <typename A>
+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<A>();
+ // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the variable 'UnnecessaryCopy2' is copy-constructed from a const reference
+ // CHECK-FIXES: const A& UnnecessaryCopy2 = templatedReference<A>();
+ UnnecessaryCopy2.constMethod();
+
+ A UnnecessaryCopy3 = Orig.template templatedAccessor<A>();
+ // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the variable 'UnnecessaryCopy3' is copy-constructed from a const reference
+ // CHECK-FIXES: const A& UnnecessaryCopy3 = Orig.template templatedAccessor<A>();
+ UnnecessaryCopy3.constMethod();
+}
+
+void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType<ExpensiveToCopyType>(); }
More information about the cfe-commits
mailing list