Skip to content

Commit badf08d

Browse files
committed
[clang-tidy] Add support for determining constness of more expressions.
This uses a more systematic approach for determining whcich `DeclRefExpr`s mutate the underlying object: Instead of using a few matchers, we walk up the AST until we find a parent that we can prove cannot change the underlying object. This allows us to handle most address taking and dereference, bindings to value and const& variables, and track constness of pointee (see changes in DeclRefExprUtilsTest.cpp). This allows supporting more patterns in `performance-unnecessary-copy-initialization`. Those two patterns are relatively common: ``` const auto e = (*vector_ptr)[i] ``` and ``` const auto e = vector_ptr->at(i); ``` In our codebase, we have around 25% additional findings from `performance-unnecessary-copy-initialization` with this change. I did not see any additional false positives.
1 parent 3f0404a commit badf08d

File tree

5 files changed

+390
-183
lines changed

5 files changed

+390
-183
lines changed

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

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -86,16 +86,22 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
8686
const auto MethodDecl =
8787
cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst())))
8888
.bind(MethodDeclId);
89-
const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
89+
const auto ReceiverExpr =
90+
ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId))));
91+
const auto OnExpr = anyOf(
92+
// Direct reference to `*this`: `a.f()` or `a->f()`.
93+
ReceiverExpr,
94+
// Access through dereference, typically used for `operator[]`: `(*a)[3]`.
95+
unaryOperator(hasOperatorName("*"), hasUnaryOperand(ReceiverExpr)));
9096
const auto ReceiverType =
9197
hasCanonicalType(recordType(hasDeclaration(namedDecl(
9298
unless(matchers::matchesAnyListedName(ExcludedContainerTypes))))));
9399

94-
return expr(anyOf(
95-
cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
96-
thisPointerType(ReceiverType)),
97-
cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
98-
hasArgument(0, hasType(ReceiverType)))));
100+
return expr(
101+
anyOf(cxxMemberCallExpr(callee(MethodDecl), on(OnExpr),
102+
thisPointerType(ReceiverType)),
103+
cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, OnExpr),
104+
hasArgument(0, hasType(ReceiverType)))));
99105
}
100106

101107
AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
@@ -136,10 +142,11 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst,
136142
static bool isInitializingVariableImmutable(
137143
const VarDecl &InitializingVar, const Stmt &BlockStmt, ASTContext &Context,
138144
const std::vector<StringRef> &ExcludedContainerTypes) {
139-
if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
145+
QualType T = InitializingVar.getType().getCanonicalType();
146+
if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context,
147+
T->isPointerType() ? 1 : 0))
140148
return false;
141149

142-
QualType T = InitializingVar.getType().getCanonicalType();
143150
// The variable is a value type and we know it is only used as const. Safe
144151
// to reference it and avoid the copy.
145152
if (!isa<ReferenceType, PointerType>(T))
@@ -273,7 +280,9 @@ void UnnecessaryCopyInitialization::check(
273280
VarDeclStmt.isSingleDecl() && !NewVar.getLocation().isMacroID();
274281
const bool IsVarUnused = isVariableUnused(NewVar, BlockStmt, *Result.Context);
275282
const bool IsVarOnlyUsedAsConst =
276-
isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context);
283+
isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context,
284+
// `NewVar` is always of non-pointer type.
285+
0);
277286
const CheckContext Context{
278287
NewVar, BlockStmt, VarDeclStmt, *Result.Context,
279288
IssueFix, IsVarUnused, IsVarOnlyUsedAsConst};

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

Lines changed: 165 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
#include "Matchers.h"
1111
#include "clang/AST/ASTContext.h"
1212
#include "clang/AST/DeclCXX.h"
13+
#include "clang/AST/ExprCXX.h"
1314
#include "clang/ASTMatchers/ASTMatchFinder.h"
15+
#include <cassert>
1416

1517
namespace clang::tidy::utils::decl_ref_expr {
1618

@@ -34,69 +36,185 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
3436
Nodes.insert(Match.getNodeAs<Node>(ID));
3537
}
3638

39+
// A matcher that matches DeclRefExprs that are used in ways such that the
40+
// underlying declaration is not modified.
41+
// If the declaration is of pointer type, `Indirections` specifies the level
42+
// of indirection of the object whose mutations we are tracking.
43+
//
44+
// For example, given:
45+
// ```
46+
// int i;
47+
// int* p;
48+
// p = &i; // (A)
49+
// *p = 3; // (B)
50+
// ```
51+
//
52+
// `declRefExpr(to(varDecl(hasName("p"))), doesNotMutateObject(0))` matches
53+
// (B), but `declRefExpr(to(varDecl(hasName("p"))), doesNotMutateObject(1))`
54+
// matches (A).
55+
//
56+
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
61+
// recursion there.
62+
// - Those that can be used to mutate the underlying object in analyzable
63+
// ways (such as taking the address or accessing a subobject). We have to
64+
// examine the parents.
65+
// - 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.
67+
68+
struct StackEntry {
69+
StackEntry(const Expr *E, int Indirections)
70+
: E(E), Indirections(Indirections) {}
71+
// The expression to analyze.
72+
const Expr *E;
73+
// The number of pointer indirections of the object being tracked (how
74+
// many times an address was taken).
75+
int Indirections;
76+
};
77+
78+
llvm::SmallVector<StackEntry, 4> Stack;
79+
Stack.emplace_back(&Node, Indirections);
80+
auto &Ctx = Finder->getASTContext();
81+
82+
while (!Stack.empty()) {
83+
const StackEntry Entry = Stack.back();
84+
Stack.pop_back();
85+
86+
// If the expression type is const-qualified at the appropriate indirection
87+
// level then we can not mutate the object.
88+
QualType Ty = Entry.E->getType().getCanonicalType();
89+
for (int I = 0; I < Entry.Indirections; ++I) {
90+
assert(Ty->isPointerType());
91+
Ty = Ty->getPointeeType().getCanonicalType();
92+
}
93+
if (Ty.isConstQualified()) {
94+
continue;
95+
}
96+
97+
// Otherwise we have to look at the parents to see how the expression is
98+
// used.
99+
const auto Parents = Ctx.getParents(*Entry.E);
100+
// Note: most nodes have a single parents, but there exist nodes that have
101+
// several parents, such as `InitListExpr` that have semantic and syntactic
102+
// forms.
103+
for (const auto &Parent : Parents) {
104+
if (Parent.get<CompoundStmt>()) {
105+
// Unused block-scope statement.
106+
continue;
107+
}
108+
const Expr *const P = Parent.get<Expr>();
109+
if (P == nullptr) {
110+
// `Parent` is not an expr (e.g. a `VarDecl`).
111+
// The case of binding to a `const&` or `const*` variable is handled by
112+
// the fact that there is going to be a `NoOp` cast to const below the
113+
// `VarDecl`, so we're not even going to get there.
114+
// The case of copying into a value-typed variable is handled by the
115+
// rvalue cast.
116+
// This triggers only when binding to a mutable reference/ptr variable.
117+
// FIXME: When we take a mutable reference we could keep checking the
118+
// new variable for const usage only.
119+
return false;
120+
}
121+
// Cosmetic nodes.
122+
if (isa<ParenExpr>(P) || isa<MaterializeTemporaryExpr>(P)) {
123+
Stack.emplace_back(P, Entry.Indirections);
124+
continue;
125+
}
126+
if (const auto *const Cast = dyn_cast<CastExpr>(P)) {
127+
switch (Cast->getCastKind()) {
128+
// NoOp casts are used to add `const`. We'll check whether adding that
129+
// const prevents modification when we process the cast.
130+
case CK_NoOp:
131+
// These do nothing w.r.t. to mutability.
132+
case CK_BaseToDerived:
133+
case CK_DerivedToBase:
134+
case CK_UncheckedDerivedToBase:
135+
case CK_Dynamic:
136+
case CK_BaseToDerivedMemberPointer:
137+
case CK_DerivedToBaseMemberPointer:
138+
Stack.emplace_back(Cast, Entry.Indirections);
139+
continue;
140+
case CK_ToVoid:
141+
case CK_PointerToBoolean:
142+
// These do not mutate the underlying variable.
143+
continue;
144+
case CK_LValueToRValue: {
145+
// An rvalue is immutable.
146+
if (Entry.Indirections == 0) {
147+
continue;
148+
}
149+
Stack.emplace_back(Cast, Entry.Indirections);
150+
continue;
151+
}
152+
default:
153+
// Bail out on casts that we cannot analyze.
154+
return false;
155+
}
156+
}
157+
if (const auto *const Member = dyn_cast<MemberExpr>(P)) {
158+
if (const auto *const Method =
159+
dyn_cast<CXXMethodDecl>(Member->getMemberDecl())) {
160+
if (!Method->isConst()) {
161+
// The method can mutate our variable.
162+
return false;
163+
}
164+
continue;
165+
}
166+
Stack.emplace_back(Member, 0);
167+
continue;
168+
}
169+
if (const auto *const Op = dyn_cast<UnaryOperator>(P)) {
170+
switch (Op->getOpcode()) {
171+
case UO_AddrOf:
172+
Stack.emplace_back(Op, Entry.Indirections + 1);
173+
continue;
174+
case UO_Deref:
175+
assert(Entry.Indirections > 0);
176+
Stack.emplace_back(Op, Entry.Indirections - 1);
177+
continue;
178+
default:
179+
// Bail out on unary operators that we cannot analyze.
180+
return false;
181+
}
182+
}
183+
184+
// Assume any other expression can modify the underlying variable.
185+
return false;
186+
}
187+
}
188+
189+
// No parent can modify the variable.
190+
return true;
191+
}
192+
37193
} // namespace
38194

39-
// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
40-
// is the a const reference or value argument to a CallExpr or CXXConstructExpr.
41195
SmallPtrSet<const DeclRefExpr *, 16>
42196
constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
43-
ASTContext &Context) {
44-
auto DeclRefToVar =
45-
declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
46-
auto MemberExprOfVar = memberExpr(hasObjectExpression(DeclRefToVar));
47-
auto DeclRefToVarOrMemberExprOfVar =
48-
stmt(anyOf(DeclRefToVar, MemberExprOfVar));
49-
auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
50-
// Match method call expressions where the variable is referenced as the this
51-
// implicit object argument and operator call expression for member operators
52-
// where the variable is the 0-th argument.
53-
auto Matches = match(
54-
findAll(expr(anyOf(
55-
cxxMemberCallExpr(ConstMethodCallee,
56-
on(DeclRefToVarOrMemberExprOfVar)),
57-
cxxOperatorCallExpr(ConstMethodCallee,
58-
hasArgument(0, DeclRefToVarOrMemberExprOfVar))))),
59-
Stmt, Context);
197+
ASTContext &Context, int Indirections) {
198+
auto Matches = match(findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl))),
199+
doesNotMutateObject(Indirections))
200+
.bind("declRef")),
201+
Stmt, Context);
60202
SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
61203
extractNodesByIdTo(Matches, "declRef", DeclRefs);
62-
auto ConstReferenceOrValue =
63-
qualType(anyOf(matchers::isReferenceToConst(),
64-
unless(anyOf(referenceType(), pointerType(),
65-
substTemplateTypeParmType()))));
66-
auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
67-
ConstReferenceOrValue,
68-
substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue))));
69-
auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
70-
DeclRefToVarOrMemberExprOfVar,
71-
parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
72-
Matches = match(findAll(invocation(UsedAsConstRefOrValueArg)), Stmt, Context);
73-
extractNodesByIdTo(Matches, "declRef", DeclRefs);
74-
// References and pointers to const assignments.
75-
Matches = match(
76-
findAll(declStmt(has(varDecl(
77-
hasType(qualType(matchers::isReferenceToConst())),
78-
hasInitializer(ignoringImpCasts(DeclRefToVarOrMemberExprOfVar)))))),
79-
Stmt, Context);
80-
extractNodesByIdTo(Matches, "declRef", DeclRefs);
81-
Matches = match(findAll(declStmt(has(varDecl(
82-
hasType(qualType(matchers::isPointerToConst())),
83-
hasInitializer(ignoringImpCasts(unaryOperator(
84-
hasOperatorName("&"),
85-
hasUnaryOperand(DeclRefToVarOrMemberExprOfVar)))))))),
86-
Stmt, Context);
87-
extractNodesByIdTo(Matches, "declRef", DeclRefs);
204+
88205
return DeclRefs;
89206
}
90207

91208
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
92-
ASTContext &Context) {
209+
ASTContext &Context, int Indirections) {
93210
// Collect all DeclRefExprs to the loop variable and all CallExprs and
94211
// CXXConstructExprs where the loop variable is used as argument to a const
95212
// reference parameter.
96213
// If the difference is empty it is safe for the loop variable to be a const
97214
// reference.
98215
auto AllDeclRefs = allDeclRefExprs(Var, Stmt, Context);
99-
auto ConstReferenceDeclRefs = constReferenceDeclRefExprs(Var, Stmt, Context);
216+
auto ConstReferenceDeclRefs =
217+
constReferenceDeclRefExprs(Var, Stmt, Context, Indirections);
100218
return isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs);
101219
}
102220

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

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,6 @@
1515

1616
namespace clang::tidy::utils::decl_ref_expr {
1717

18-
/// Returns true if all ``DeclRefExpr`` to the variable within ``Stmt``
19-
/// do not modify it.
20-
///
21-
/// Returns ``true`` if only const methods or operators are called on the
22-
/// variable or the variable is a const reference or value argument to a
23-
/// ``callExpr()``.
24-
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
25-
ASTContext &Context);
26-
2718
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``.
2819
llvm::SmallPtrSet<const DeclRefExpr *, 16>
2920
allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context);
@@ -34,9 +25,31 @@ allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context);
3425

3526
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt`` where
3627
/// ``VarDecl`` is guaranteed to be accessed in a const fashion.
28+
///
29+
/// If ``VarDecl`` is of pointer type, ``Indirections`` specifies the level
30+
/// of indirection of the object whose mutations we are tracking.
31+
///
32+
/// For example, given:
33+
/// ```
34+
/// int i;
35+
/// int* p;
36+
/// p = &i; // (A)
37+
/// *p = 3; // (B)
38+
/// ```
39+
///
40+
/// - `constReferenceDeclRefExprs(P, Stmt, Context, 0)` returns the reference
41+
// to `p` in (B): the pointee is modified, but the pointer is not;
42+
/// - `constReferenceDeclRefExprs(P, Stmt, Context, 1)` returns the reference
43+
// to `p` in (A): the pointee is modified, but the pointer is not;
3744
llvm::SmallPtrSet<const DeclRefExpr *, 16>
3845
constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
39-
ASTContext &Context);
46+
ASTContext &Context, int Indirections);
47+
48+
/// Returns true if all ``DeclRefExpr`` to the variable within ``Stmt``
49+
/// do not modify it.
50+
/// See `constReferenceDeclRefExprs` for the meaning of ``Indirections``.
51+
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
52+
ASTContext &Context, int Indirections);
4053

4154
/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-constructor
4255
/// call expression within ``Decl``.

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

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ struct Container {
4141
ConstIterator<T> end() const;
4242
void nonConstMethod();
4343
bool constMethod() const;
44+
45+
const T& at(int) const;
46+
T& at(int);
47+
4448
};
4549

4650
using ExpensiveToCopyContainerAlias = Container<ExpensiveToCopyType>;
@@ -225,25 +229,25 @@ void PositiveOperatorCallConstValueParamAlias(const ExpensiveToCopyContainerAlia
225229
VarCopyConstructed.constMethod();
226230
}
227231

228-
void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType>* C) {
232+
void PositiveOperatorCallConstPtrParam(const Container<ExpensiveToCopyType>* C) {
229233
const auto AutoAssigned = (*C)[42];
230-
// TODO-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
231-
// TODO-FIXES: const auto& AutoAssigned = (*C)[42];
234+
// CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
235+
// CHECK-FIXES: const auto& AutoAssigned = (*C)[42];
232236
AutoAssigned.constMethod();
233237

234238
const auto AutoCopyConstructed((*C)[42]);
235-
// TODO-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
236-
// TODO-FIXES: const auto& AutoCopyConstructed((*C)[42]);
239+
// CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
240+
// CHECK-FIXES: const auto& AutoCopyConstructed((*C)[42]);
237241
AutoCopyConstructed.constMethod();
238242

239-
const ExpensiveToCopyType VarAssigned = C->operator[](42);
240-
// TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
241-
// TODO-FIXES: const ExpensiveToCopyType& VarAssigned = C->operator[](42);
243+
const ExpensiveToCopyType VarAssigned = C->at(42);
244+
// CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
245+
// CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C->at(42);
242246
VarAssigned.constMethod();
243247

244-
const ExpensiveToCopyType VarCopyConstructed(C->operator[](42));
245-
// TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
246-
// TODO-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C->operator[](42));
248+
const ExpensiveToCopyType VarCopyConstructed(C->at(42));
249+
// CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
250+
// CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C->at(42));
247251
VarCopyConstructed.constMethod();
248252
}
249253

@@ -876,3 +880,4 @@ void negativeNonConstMemberExpr() {
876880
mutate(&Copy.Member);
877881
}
878882
}
883+

0 commit comments

Comments
 (0)