flx updated this revision to Diff 360868.
flx added a comment.
Addressed review feedback.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106011/new/
https://reviews.llvm.org/D106011
Files:
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -17,6 +17,9 @@
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 @@
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>(); }
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -27,6 +27,8 @@
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 @@
// 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 @@
// 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 @@
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 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<FunctionDecl>(FunctionDeclId)) {
+ return FuncDecl->getReturnType();
+ }
+ const auto *MethodDecl = Nodes.getNodeAs<CXXMethodDecl>(MethodDeclId);
+ return MethodDecl->getReturnType();
+}
+
} // namespace
UnnecessaryCopyInitialization::UnnecessaryCopyInitialization(
@@ -222,6 +265,16 @@
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);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
