Skip to content

Commit 415a82c

Browse files
authored
[clang-tidy] doesNotMutateObject: Handle calls to member functions … (#94362)
…and operators that have non-const overloads. This allows `unnecessary-copy-initialization` to warn on more cases. The common case is a class with a a set of const/non-sconst overloads (e.g. std::vector::operator[]). ``` void F() { std::vector<Expensive> v; // ... const Expensive e = v[i]; } ```
1 parent c9fd7b1 commit 415a82c

File tree

5 files changed

+247
-27
lines changed

5 files changed

+247
-27
lines changed

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -75,16 +75,16 @@ void recordRemoval(const DeclStmt &Stmt, ASTContext &Context,
7575
}
7676
}
7777

78-
AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
78+
AST_MATCHER_FUNCTION_P(StatementMatcher,
79+
isRefReturningMethodCallWithConstOverloads,
7980
std::vector<StringRef>, ExcludedContainerTypes) {
8081
// Match method call expressions where the `this` argument is only used as
81-
// const, this will be checked in `check()` part. This returned const
82-
// reference is highly likely to outlive the local const reference of the
83-
// variable being declared. The assumption is that the const reference being
84-
// returned either points to a global static variable or to a member of the
85-
// called object.
82+
// const, this will be checked in `check()` part. This returned reference is
83+
// highly likely to outlive the local const reference of the variable being
84+
// declared. The assumption is that the reference being returned either points
85+
// to a global static variable or to a member of the called object.
8686
const auto MethodDecl =
87-
cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst())))
87+
cxxMethodDecl(returns(hasCanonicalType(referenceType())))
8888
.bind(MethodDeclId);
8989
const auto ReceiverExpr =
9090
ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId))));
@@ -121,7 +121,7 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst,
121121
declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId)));
122122
return expr(
123123
anyOf(isConstRefReturningFunctionCall(),
124-
isConstRefReturningMethodCall(ExcludedContainerTypes),
124+
isRefReturningMethodCallWithConstOverloads(ExcludedContainerTypes),
125125
ignoringImpCasts(OldVarDeclRef),
126126
ignoringImpCasts(unaryOperator(hasOperatorName("&"),
127127
hasUnaryOperand(OldVarDeclRef)))));
@@ -259,10 +259,11 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
259259
.bind("blockStmt");
260260
};
261261

262-
Finder->addMatcher(LocalVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(),
263-
isConstRefReturningMethodCall(
264-
ExcludedContainerTypes))),
265-
this);
262+
Finder->addMatcher(
263+
LocalVarCopiedFrom(anyOf(
264+
isConstRefReturningFunctionCall(),
265+
isRefReturningMethodCallWithConstOverloads(ExcludedContainerTypes))),
266+
this);
266267

267268
Finder->addMatcher(LocalVarCopiedFrom(declRefExpr(
268269
to(varDecl(hasLocalStorage()).bind(OldVarDeclId)))),

clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp

Lines changed: 161 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,116 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
3636
Nodes.insert(Match.getNodeAs<Node>(ID));
3737
}
3838

39+
// Returns true if both types refer to the same type,
40+
// ignoring the const-qualifier.
41+
bool isSameTypeIgnoringConst(QualType A, QualType B) {
42+
A = A.getCanonicalType();
43+
B = B.getCanonicalType();
44+
A.addConst();
45+
B.addConst();
46+
return A == B;
47+
}
48+
49+
// Returns true if `D` and `O` have the same parameter types.
50+
bool hasSameParameterTypes(const CXXMethodDecl &D, const CXXMethodDecl &O) {
51+
if (D.getNumParams() != O.getNumParams())
52+
return false;
53+
for (int I = 0, E = D.getNumParams(); I < E; ++I) {
54+
if (!isSameTypeIgnoringConst(D.getParamDecl(I)->getType(),
55+
O.getParamDecl(I)->getType()))
56+
return false;
57+
}
58+
return true;
59+
}
60+
61+
// If `D` has a const-qualified overload with otherwise identical
62+
// ref-qualifiers and parameter types, returns that overload.
63+
const CXXMethodDecl *findConstOverload(const CXXMethodDecl &D) {
64+
assert(!D.isConst());
65+
66+
DeclContext::lookup_result LookupResult =
67+
D.getParent()->lookup(D.getNameInfo().getName());
68+
if (LookupResult.isSingleResult()) {
69+
// No overload.
70+
return nullptr;
71+
}
72+
for (const Decl *Overload : LookupResult) {
73+
const auto *O = dyn_cast<CXXMethodDecl>(Overload);
74+
if (O && !O->isDeleted() && O->isConst() &&
75+
O->getRefQualifier() == D.getRefQualifier() &&
76+
hasSameParameterTypes(D, *O))
77+
return O;
78+
}
79+
return nullptr;
80+
}
81+
82+
// Returns true if both types are pointers or reference to the same type,
83+
// ignoring the const-qualifier.
84+
bool pointsToSameTypeIgnoringConst(QualType A, QualType B) {
85+
assert(A->isPointerType() || A->isReferenceType());
86+
assert(B->isPointerType() || B->isReferenceType());
87+
return isSameTypeIgnoringConst(A->getPointeeType(), B->getPointeeType());
88+
}
89+
90+
// Return true if non-const member function `M` likely does not mutate `*this`.
91+
//
92+
// Note that if the member call selects a method/operator `f` that
93+
// is not const-qualified, then we also consider that the object is
94+
// not mutated if:
95+
// - (A) there is a const-qualified overload `cf` of `f` that has
96+
// the
97+
// same ref-qualifiers;
98+
// - (B) * `f` returns a value, or
99+
// * if `f` returns a `T&`, `cf` returns a `const T&` (up to
100+
// possible aliases such as `reference` and
101+
// `const_reference`), or
102+
// * if `f` returns a `T*`, `cf` returns a `const T*` (up to
103+
// possible aliases).
104+
// - (C) the result of the call is not mutated.
105+
//
106+
// The assumption that `cf` has the same semantics as `f`.
107+
// For example:
108+
// - In `std::vector<T> v; const T t = v[...];`, we consider that
109+
// expression `v[...]` does not mutate `v` as
110+
// `T& std::vector<T>::operator[]` has a const overload
111+
// `const T& std::vector<T>::operator[] const`, and the
112+
// result expression of type `T&` is only used as a `const T&`;
113+
// - In `std::map<K, V> m; V v = m.at(...);`, we consider
114+
// `m.at(...)` to be an immutable access for the same reason.
115+
// However:
116+
// - In `std::map<K, V> m; const V v = m[...];`, We consider that
117+
// `m[...]` mutates `m` as `V& std::map<K, V>::operator[]` does
118+
// not have a const overload.
119+
// - In `std::vector<T> v; T& t = v[...];`, we consider that
120+
// expression `v[...]` mutates `v` as the result is kept as a
121+
// mutable reference.
122+
//
123+
// This function checks (A) ad (B), but the caller should make sure that the
124+
// object is not mutated through the return value.
125+
bool isLikelyShallowConst(const CXXMethodDecl &M) {
126+
assert(!M.isConst());
127+
// The method can mutate our variable.
128+
129+
// (A)
130+
const CXXMethodDecl *ConstOverload = findConstOverload(M);
131+
if (ConstOverload == nullptr) {
132+
return false;
133+
}
134+
135+
// (B)
136+
const QualType CallTy = M.getReturnType().getCanonicalType();
137+
const QualType OverloadTy = ConstOverload->getReturnType().getCanonicalType();
138+
if (CallTy->isReferenceType()) {
139+
return OverloadTy->isReferenceType() &&
140+
pointsToSameTypeIgnoringConst(CallTy, OverloadTy);
141+
}
142+
if (CallTy->isPointerType()) {
143+
return OverloadTy->isPointerType() &&
144+
pointsToSameTypeIgnoringConst(CallTy, OverloadTy);
145+
}
146+
return isSameTypeIgnoringConst(CallTy, OverloadTy);
147+
}
148+
39149
// A matcher that matches DeclRefExprs that are used in ways such that the
40150
// underlying declaration is not modified.
41151
// If the declaration is of pointer type, `Indirections` specifies the level
@@ -54,16 +164,15 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
54164
// matches (A).
55165
//
56166
AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
57-
// We walk up the parents of the DeclRefExpr recursively until we end up on a
58-
// parent that cannot modify the underlying object. There are a few kinds of
59-
// expressions:
60-
// - Those that cannot be used to mutate the underlying object. We can stop
167+
// We walk up the parents of the DeclRefExpr recursively. There are a few
168+
// kinds of expressions:
169+
// - Those that cannot be used to mutate the underlying variable. We can stop
61170
// recursion there.
62-
// - Those that can be used to mutate the underlying object in analyzable
171+
// - Those that can be used to mutate the underlying variable in analyzable
63172
// ways (such as taking the address or accessing a subobject). We have to
64173
// examine the parents.
65174
// - Those that we don't know how to analyze. In that case we stop there and
66-
// we assume that they can mutate the underlying expression.
175+
// we assume that they can modify the expression.
67176

68177
struct StackEntry {
69178
StackEntry(const Expr *E, int Indirections)
@@ -90,7 +199,7 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
90199
assert(Ty->isPointerType());
91200
Ty = Ty->getPointeeType().getCanonicalType();
92201
}
93-
if (Ty.isConstQualified())
202+
if (Ty->isVoidType() || Ty.isConstQualified())
94203
continue;
95204

96205
// Otherwise we have to look at the parents to see how the expression is
@@ -159,11 +268,56 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
159268
// The method call cannot mutate our variable.
160269
continue;
161270
}
271+
if (isLikelyShallowConst(*Method)) {
272+
// We still have to check that the object is not modified through
273+
// the method's return value (C).
274+
const auto MemberParents = Ctx.getParents(*Member);
275+
assert(MemberParents.size() == 1);
276+
const auto *Call = MemberParents[0].get<CallExpr>();
277+
// If `o` is an object of class type and `f` is a member function,
278+
// then `o.f` has to be used as part of a call expression.
279+
assert(Call != nullptr && "member function has to be called");
280+
Stack.emplace_back(
281+
Call,
282+
Method->getReturnType().getCanonicalType()->isPointerType()
283+
? 1
284+
: 0);
285+
continue;
286+
}
162287
return false;
163288
}
164289
Stack.emplace_back(Member, 0);
165290
continue;
166291
}
292+
if (const auto *const OpCall = dyn_cast<CXXOperatorCallExpr>(P)) {
293+
// Operator calls have function call syntax. The `*this` parameter
294+
// is the first parameter.
295+
if (OpCall->getNumArgs() == 0 || OpCall->getArg(0) != Entry.E) {
296+
return false;
297+
}
298+
const auto *const Method =
299+
dyn_cast<CXXMethodDecl>(OpCall->getDirectCallee());
300+
301+
if (Method == nullptr) {
302+
// This is not a member operator. Typically, a friend operator. These
303+
// are handled like function calls.
304+
return false;
305+
}
306+
307+
if (Method->isConst() || Method->isStatic()) {
308+
continue;
309+
}
310+
if (isLikelyShallowConst(*Method)) {
311+
// We still have to check that the object is not modified through
312+
// the operator's return value (C).
313+
Stack.emplace_back(
314+
OpCall,
315+
Method->getReturnType().getCanonicalType()->isPointerType() ? 1
316+
: 0);
317+
continue;
318+
}
319+
return false;
320+
}
167321

168322
if (const auto *const Op = dyn_cast<UnaryOperator>(P)) {
169323
switch (Op->getOpcode()) {

clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -376,8 +376,10 @@ Changes in existing checks
376376
- Improved :doc:`performance-unnecessary-copy-initialization
377377
<clang-tidy/checks/performance/unnecessary-copy-initialization>` check by
378378
detecting more cases of constant access. In particular, pointers can be
379-
analyzed, se the check now handles the common patterns
379+
analyzed, so the check now handles the common patterns
380380
`const auto e = (*vector_ptr)[i]` and `const auto e = vector_ptr->at(i);`.
381+
Calls to mutable function where there exists a `const` overload are also
382+
handled.
381383

382384
- Improved :doc:`readability-avoid-return-with-void-value
383385
<clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding

clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ struct ExpensiveToCopyType {
3232

3333
template <typename T>
3434
struct Container {
35+
using reference = T&;
36+
using const_reference = const T&;
37+
3538
bool empty() const;
3639
const T& operator[](int) const;
3740
const T& operator[](int);
@@ -42,8 +45,8 @@ struct Container {
4245
void nonConstMethod();
4346
bool constMethod() const;
4447

45-
const T& at(int) const;
46-
T& at(int);
48+
reference at(int) const;
49+
const_reference at(int);
4750

4851
};
4952

@@ -207,6 +210,28 @@ void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType> C)
207210
VarCopyConstructed.constMethod();
208211
}
209212

213+
void PositiveOperatorValueParam(Container<ExpensiveToCopyType> C) {
214+
const auto AutoAssigned = C[42];
215+
// CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
216+
// CHECK-FIXES: const auto& AutoAssigned = C[42];
217+
AutoAssigned.constMethod();
218+
219+
const auto AutoCopyConstructed(C[42]);
220+
// CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
221+
// CHECK-FIXES: const auto& AutoCopyConstructed(C[42]);
222+
AutoCopyConstructed.constMethod();
223+
224+
const ExpensiveToCopyType VarAssigned = C.at(42);
225+
// CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
226+
// CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C.at(42);
227+
VarAssigned.constMethod();
228+
229+
const ExpensiveToCopyType VarCopyConstructed(C.at(42));
230+
// CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
231+
// CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C.at(42));
232+
VarCopyConstructed.constMethod();
233+
}
234+
210235
void PositiveOperatorCallConstValueParamAlias(const ExpensiveToCopyContainerAlias C) {
211236
const auto AutoAssigned = C[42];
212237
// CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'

0 commit comments

Comments
 (0)