Skip to content

Commit 950760d

Browse files
goldvitalyyuxuanchen1997
goldvitaly
authored and
yuxuanchen1997
committed
[clang-tidy] Allow unnecessary-value-param to match templated functions including lambdas with auto. (#97767)
Clang-Tidy unnecessary-value-param value param will be triggered for templated functions if at least one instantiontion with expensive to copy type is present in translation unit. It is relatively common mistake to write lambda functions with auto arguments for expensive to copy types.
1 parent 6294771 commit 950760d

File tree

5 files changed

+109
-50
lines changed

5 files changed

+109
-50
lines changed

‎clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
7575
functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()),
7676
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
7777
has(typeLoc(forEach(ExpensiveValueParamDecl))),
78-
unless(isInstantiated()), decl().bind("functionDecl"))),
78+
decl().bind("functionDecl"))),
7979
this);
8080
}
8181

@@ -133,12 +133,11 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
133133
// 2. the function is virtual as it might break overrides
134134
// 3. the function is referenced outside of a call expression within the
135135
// compilation unit as the signature change could introduce build errors.
136-
// 4. the function is a primary template or an explicit template
137-
// specialization.
136+
// 4. the function is an explicit template/ specialization.
138137
const auto *Method = llvm::dyn_cast<CXXMethodDecl>(Function);
139138
if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
140139
isReferencedOutsideOfCallExpr(*Function, *Result.Context) ||
141-
(Function->getTemplatedKind() != FunctionDecl::TK_NonTemplate))
140+
Function->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
142141
return;
143142
for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
144143
FunctionDecl = FunctionDecl->getPreviousDecl()) {

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,12 @@ Changes in existing checks
437437
Calls to mutable function where there exists a `const` overload are also
438438
handled. Fix crash in the case of a non-member operator call.
439439

440+
- Improved :doc:`performance-unnecessary-value-param
441+
<clang-tidy/checks/performance/unnecessary-value-param>` check
442+
detecting more cases for template functions including lambdas with ``auto``.
443+
E.g., ``std::sort(a.begin(), a.end(), [](auto x, auto y) { return a > b; });``
444+
will be detected for expensive to copy types.
445+
440446
- Improved :doc:`readability-avoid-return-with-void-value
441447
<clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding
442448
fix-its.

‎clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-delayed.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,8 @@ struct PositiveConstValueConstructor {
6969

7070
template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
7171
// CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
72-
// CHECK-FIXES-NOT: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
72+
// CHECK-MESSAGES: [[@LINE-2]]:95: warning: the parameter 'V'
73+
// CHECK-FIXES: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, const T& V) {
7374
}
7475

7576
void instantiated() {

‎clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-templates.cpp

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// RUN: %check_clang_tidy -std=c++14-or-later %s performance-unnecessary-value-param %t
2+
3+
struct ExpensiveToCopyType {
4+
virtual ~ExpensiveToCopyType();
5+
};
6+
7+
template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
8+
// CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
9+
// CHECK-MESSAGES: [[@LINE-2]]:95: warning: the parameter 'V'
10+
// CHECK-FIXES: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, const T& V) {
11+
}
12+
13+
void instantiatedWithExpensiveValue() {
14+
templateWithNonTemplatizedParameter(
15+
ExpensiveToCopyType(), ExpensiveToCopyType());
16+
templateWithNonTemplatizedParameter(ExpensiveToCopyType(), 5);
17+
}
18+
19+
template <typename T> void templateWithNonTemplatizedParameterCheapTemplate(const ExpensiveToCopyType S, T V) {
20+
// CHECK-MESSAGES: [[@LINE-1]]:103: warning: the const qualified parameter 'S'
21+
// CHECK-FIXES: template <typename T> void templateWithNonTemplatizedParameterCheapTemplate(const ExpensiveToCopyType& S, T V) {
22+
}
23+
24+
void instantiatedWithCheapValue() {
25+
templateWithNonTemplatizedParameterCheapTemplate(ExpensiveToCopyType(), 5);
26+
}
27+
28+
template <typename T> void nonInstantiatedTemplateWithConstValue(const T S) {}
29+
template <typename T> void nonInstantiatedTemplateWithNonConstValue(T S) {}
30+
31+
template <typename T> void instantiatedTemplateSpecialization(T NoSpecS) {}
32+
template <> void instantiatedTemplateSpecialization<int>(int SpecSInt) {}
33+
// Updating template specialization would also require to update the main
34+
// template and other specializations. Such specializations may be
35+
// spreaded across different translation units.
36+
// For that reason we only issue a warning, but do not propose fixes.
37+
template <>
38+
void instantiatedTemplateSpecialization<ExpensiveToCopyType>(
39+
ExpensiveToCopyType SpecSExpensiveToCopy) {
40+
// CHECK-MESSAGES: [[@LINE-1]]:25: warning: the parameter 'SpecSExpensiveToCopy'
41+
// CHECK-FIXES-NOT: const T& NoSpecS
42+
// CHECK-FIXES-NOT: const int& SpecSInt
43+
// CHECK-FIXES-NOT: const ExpensiveToCopyType& SpecSExpensiveToCopy
44+
}
45+
46+
void instantiatedTemplateSpecialization() {
47+
instantiatedTemplateSpecialization(ExpensiveToCopyType());
48+
}
49+
50+
template <typename T> void instantiatedTemplateWithConstValue(const T S) {
51+
// CHECK-MESSAGES: [[@LINE-1]]:71: warning: the const qualified parameter 'S'
52+
// CHECK-FIXES: template <typename T> void instantiatedTemplateWithConstValue(const T& S) {
53+
}
54+
55+
void instantiatedConstValue() {
56+
instantiatedTemplateWithConstValue(ExpensiveToCopyType());
57+
}
58+
59+
template <typename T> void instantiatedTemplateWithNonConstValue(T S) {
60+
// CHECK-MESSAGES: [[@LINE-1]]:68: warning: the parameter 'S'
61+
// CHECK-FIXES: template <typename T> void instantiatedTemplateWithNonConstValue(const T& S) {
62+
}
63+
64+
void instantiatedNonConstValue() {
65+
instantiatedTemplateWithNonConstValue(ExpensiveToCopyType());
66+
}
67+
68+
void lambdaConstValue() {
69+
auto fn = [](const ExpensiveToCopyType S) {
70+
// CHECK-MESSAGES: [[@LINE-1]]:42: warning: the const qualified parameter 'S'
71+
// CHECK-FIXES: auto fn = [](const ExpensiveToCopyType& S) {
72+
};
73+
fn(ExpensiveToCopyType());
74+
}
75+
76+
void lambdaNonConstValue() {
77+
auto fn = [](ExpensiveToCopyType S) {
78+
// CHECK-MESSAGES: [[@LINE-1]]:36: warning: the parameter 'S'
79+
// CHECK-FIXES: auto fn = [](const ExpensiveToCopyType& S) {
80+
};
81+
fn(ExpensiveToCopyType());
82+
}
83+
84+
void lambdaConstAutoValue() {
85+
auto fn = [](const auto S) {
86+
// CHECK-MESSAGES: [[@LINE-1]]:27: warning: the const qualified parameter 'S'
87+
// CHECK-FIXES: auto fn = [](const auto& S) {
88+
};
89+
fn(ExpensiveToCopyType());
90+
}
91+
92+
void lambdaNonConstAutoValue() {
93+
auto fn = [](auto S) {
94+
// CHECK-MESSAGES: [[@LINE-1]]:21: warning: the parameter 'S'
95+
// CHECK-FIXES: auto fn = [](const auto& S) {
96+
};
97+
fn(ExpensiveToCopyType());
98+
}

‎clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp

Lines changed: 0 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -107,19 +107,6 @@ struct PositiveConstValueConstructor {
107107
// CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {}
108108
};
109109

110-
template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
111-
// CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
112-
// CHECK-FIXES-NOT: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
113-
}
114-
115-
void instantiated() {
116-
templateWithNonTemplatizedParameter(ExpensiveToCopyType(), ExpensiveToCopyType());
117-
templateWithNonTemplatizedParameter(ExpensiveToCopyType(), 5);
118-
}
119-
120-
template <typename T> void negativeTemplateType(const T V) {
121-
}
122-
123110
void negativeArray(const ExpensiveToCopyType[]) {
124111
}
125112

@@ -370,35 +357,3 @@ void fun() {
370357
ExpensiveToCopyType E;
371358
NegativeUsingConstructor S(E);
372359
}
373-
374-
template<typename T>
375-
void templateFunction(T) {
376-
}
377-
378-
template<>
379-
void templateFunction<ExpensiveToCopyType>(ExpensiveToCopyType E) {
380-
// CHECK-MESSAGES: [[@LINE-1]]:64: warning: the parameter 'E' is copied
381-
// CHECK-FIXES: void templateFunction<ExpensiveToCopyType>(ExpensiveToCopyType E) {
382-
E.constReference();
383-
}
384-
385-
template <class T>
386-
T templateSpecializationFunction(ExpensiveToCopyType E) {
387-
// CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
388-
// CHECK-FIXES-NOT: T templateSpecializationFunction(const ExpensiveToCopyType& E) {
389-
return T();
390-
}
391-
392-
template <>
393-
bool templateSpecializationFunction(ExpensiveToCopyType E) {
394-
// CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied
395-
// CHECK-FIXES-NOT: bool templateSpecializationFunction(const ExpensiveToCopyType& E) {
396-
return true;
397-
}
398-
399-
template <>
400-
int templateSpecializationFunction(ExpensiveToCopyType E) {
401-
// CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied
402-
// CHECK-FIXES-NOT: int templateSpecializationFunction(const ExpensiveToCopyType& E) {
403-
return 0;
404-
}

0 commit comments

Comments
 (0)