Skip to content

Commit c6f6885

Browse files
floventflovent
committed
Refactor the way how we match Two VarDecls
1 parent d2eb66b commit c6f6885

File tree

2 files changed

+115
-145
lines changed

2 files changed

+115
-145
lines changed

‎clang-tools-extra/clang-tidy/modernize/UseStructuredBindingCheck.cpp‎

Lines changed: 95 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -20,54 +20,74 @@ static constexpr llvm::StringLiteral PairDeclName = "PairVarD";
2020
static constexpr llvm::StringLiteral PairVarTypeName = "PairVarType";
2121
static constexpr llvm::StringLiteral FirstVarDeclName = "FirstVarDecl";
2222
static constexpr llvm::StringLiteral SecondVarDeclName = "SecondVarDecl";
23-
static constexpr llvm::StringLiteral FirstDeclStmtName = "FirstDeclStmt";
24-
static constexpr llvm::StringLiteral SecondDeclStmtName = "SecondDeclStmt";
23+
static constexpr llvm::StringLiteral BeginDeclStmtName = "BeginDeclStmt";
24+
static constexpr llvm::StringLiteral EndDeclStmtName = "EndDeclStmt";
2525
static constexpr llvm::StringLiteral FirstTypeName = "FirstType";
2626
static constexpr llvm::StringLiteral SecondTypeName = "SecondType";
2727
static constexpr llvm::StringLiteral ScopeBlockName = "ScopeBlock";
2828
static constexpr llvm::StringLiteral StdTieAssignStmtName = "StdTieAssign";
2929
static constexpr llvm::StringLiteral StdTieExprName = "StdTieExpr";
3030
static constexpr llvm::StringLiteral ForRangeStmtName = "ForRangeStmt";
3131

32-
/// Try to match exactly two VarDecl inside two DeclStmts, and set binding for
33-
/// the used DeclStmts.
34-
static bool
35-
matchTwoVarDecl(const DeclStmt *DS1, const DeclStmt *DS2,
36-
ast_matchers::internal::Matcher<VarDecl> InnerMatcher1,
37-
ast_matchers::internal::Matcher<VarDecl> InnerMatcher2,
38-
internal::ASTMatchFinder *Finder,
39-
internal::BoundNodesTreeBuilder *Builder) {
40-
SmallVector<std::pair<const VarDecl *, const DeclStmt *>, 2> Vars;
41-
auto CollectVarsInDeclStmt = [&Vars](const DeclStmt *DS) -> bool {
42-
if (!DS)
43-
return true;
32+
/// Matches a sequence of VarDecls matching the inner matchers, starting from
33+
/// the \p Iter to \p EndIter and set bindings for the first DeclStmt and the
34+
/// last DeclStmt if matched.
35+
///
36+
/// \p Backwards indicates whether to match the VarDecls in reverse order.
37+
template <typename Iterator>
38+
static bool matchNVarDeclStartingWith(
39+
Iterator Iter, Iterator EndIter,
40+
ArrayRef<ast_matchers::internal::Matcher<VarDecl>> InnerMatchers,
41+
ast_matchers::internal::ASTMatchFinder *Finder,
42+
ast_matchers::internal::BoundNodesTreeBuilder *Builder,
43+
bool Backwards = false) {
44+
const DeclStmt *BeginDS = nullptr;
45+
const DeclStmt *EndDS = nullptr;
46+
size_t N = InnerMatchers.size();
47+
size_t Count = 0;
48+
for (; Iter != EndIter; ++Iter) {
49+
EndDS = dyn_cast<DeclStmt>(*Iter);
50+
if (!EndDS)
51+
break;
4452

45-
for (const auto *VD : DS->decls()) {
46-
if (Vars.size() == 2)
47-
return false;
53+
if (!BeginDS)
54+
BeginDS = EndDS;
4855

49-
if (const auto *Var = dyn_cast<VarDecl>(VD))
50-
Vars.emplace_back(Var, DS);
51-
else
56+
auto Matches = [&](const Decl *VD) {
57+
if (Count == N)
5258
return false;
53-
}
5459

55-
return true;
56-
};
57-
58-
// If we already get two VarDecls, then don't need to collect from DS2.
59-
if (!CollectVarsInDeclStmt(DS1) ||
60-
(Vars.size() != 2 && !CollectVarsInDeclStmt(DS2)) || Vars.size() != 2)
61-
return false;
60+
if (const auto *Var = dyn_cast<VarDecl>(VD);
61+
Var && InnerMatchers[Backwards ? N - Count - 1 : Count].matches(
62+
*Var, Finder, Builder)) {
63+
++Count;
64+
return true;
65+
}
66+
67+
return false;
68+
};
69+
70+
if (Backwards) {
71+
for (const auto *VD : llvm::reverse(EndDS->decls())) {
72+
if (!Matches(VD))
73+
return false;
74+
}
75+
} else {
76+
for (const auto *VD : EndDS->decls()) {
77+
if (!Matches(VD))
78+
return false;
79+
}
80+
}
6281

63-
if (InnerMatcher1.matches(*Vars[0].first, Finder, Builder) &&
64-
InnerMatcher2.matches(*Vars[1].first, Finder, Builder)) {
65-
Builder->setBinding(FirstDeclStmtName,
66-
clang::DynTypedNode::create(*Vars[0].second));
67-
if (Vars[0].second != Vars[1].second)
68-
Builder->setBinding(SecondDeclStmtName,
69-
clang::DynTypedNode::create(*Vars[1].second));
70-
return true;
82+
// All the matchers is satisfied in those DeclStmts.
83+
if (Count == N) {
84+
Builder->setBinding(
85+
BeginDeclStmtName,
86+
clang::DynTypedNode::create(Backwards ? *EndDS : *BeginDS));
87+
Builder->setBinding(EndDeclStmtName, clang::DynTypedNode::create(
88+
Backwards ? *BeginDS : *EndDS));
89+
return true;
90+
}
7191
}
7292

7393
return false;
@@ -84,12 +104,11 @@ enum TransferType : uint8_t {
84104
};
85105

86106
/// Matches a Stmt whose parent is a CompoundStmt, and which is directly
87-
/// following two VarDecls matching the inner matcher, at the same time set
88-
/// binding for the CompoundStmt.
89-
AST_MATCHER_P2(Stmt, hasPreTwoVarDecl, ast_matchers::internal::Matcher<VarDecl>,
90-
InnerMatcher1, ast_matchers::internal::Matcher<VarDecl>,
91-
InnerMatcher2) {
92-
DynTypedNodeList Parents = Finder->getASTContext().getParents(Node);
107+
/// following two VarDecls matching the inner matcher.
108+
AST_MATCHER_P(Stmt, hasPreTwoVarDecl,
109+
llvm::SmallVector<ast_matchers::internal::Matcher<VarDecl>>,
110+
InnerMatchers) {
111+
const DynTypedNodeList Parents = Finder->getASTContext().getParents(Node);
93112
if (Parents.size() != 1)
94113
return false;
95114

@@ -99,34 +118,16 @@ AST_MATCHER_P2(Stmt, hasPreTwoVarDecl, ast_matchers::internal::Matcher<VarDecl>,
99118

100119
const auto I = llvm::find(llvm::reverse(C->body()), &Node);
101120
assert(I != C->body_rend() && "C is parent of Node");
102-
if ((I + 1) == C->body_rend())
103-
return false;
104-
105-
const auto *DS2 = dyn_cast<DeclStmt>(*(I + 1));
106-
if (!DS2)
107-
return false;
108-
109-
const DeclStmt *DS1 = (!DS2->isSingleDecl() || ((I + 2) == C->body_rend())
110-
? nullptr
111-
: dyn_cast<DeclStmt>(*(I + 2)));
112-
if (DS1 && !DS1->isSingleDecl())
113-
return false;
114-
115-
if (matchTwoVarDecl(DS1, DS2, InnerMatcher1, InnerMatcher2, Finder,
116-
Builder)) {
117-
Builder->setBinding(ScopeBlockName, clang::DynTypedNode::create(*C));
118-
return true;
119-
}
120-
121-
return false;
121+
return matchNVarDeclStartingWith(I + 1, C->body_rend(), InnerMatchers, Finder,
122+
Builder, true);
122123
}
123124

124125
/// Matches a Stmt whose parent is a CompoundStmt, and which is directly
125-
/// followed by two VarDecls matching the inner matcher, at the same time set
126-
/// binding for the CompoundStmt.
127-
AST_MATCHER_P2(Stmt, hasNextTwoVarDecl,
128-
ast_matchers::internal::Matcher<VarDecl>, InnerMatcher1,
129-
ast_matchers::internal::Matcher<VarDecl>, InnerMatcher2) {
126+
/// followed by two VarDecls matching the inner matcher.
127+
AST_MATCHER_P(Stmt, hasNextTwoVarDecl,
128+
llvm::SmallVector<ast_matchers::internal::Matcher<VarDecl>>,
129+
InnerMatchers) {
130+
130131
const DynTypedNodeList Parents = Finder->getASTContext().getParents(Node);
131132
if (Parents.size() != 1)
132133
return false;
@@ -137,33 +138,17 @@ AST_MATCHER_P2(Stmt, hasNextTwoVarDecl,
137138

138139
const auto *I = llvm::find(C->body(), &Node);
139140
assert(I != C->body_end() && "C is parent of Node");
140-
if ((I + 1) == C->body_end())
141-
return false;
142-
143-
if (matchTwoVarDecl(
144-
dyn_cast<DeclStmt>(*(I + 1)),
145-
((I + 2) == C->body_end() ? nullptr : dyn_cast<DeclStmt>(*(I + 2))),
146-
InnerMatcher1, InnerMatcher2, Finder, Builder)) {
147-
Builder->setBinding(ScopeBlockName, clang::DynTypedNode::create(*C));
148-
return true;
149-
}
150-
151-
return false;
141+
return matchNVarDeclStartingWith(I + 1, C->body_end(), InnerMatchers, Finder,
142+
Builder);
152143
}
153144

154145
/// Matches a CompoundStmt which has two VarDecls matching the inner matcher in
155146
/// the beginning.
156-
AST_MATCHER_P2(CompoundStmt, hasFirstTwoVarDecl,
157-
ast_matchers::internal::Matcher<VarDecl>, InnerMatcher1,
158-
ast_matchers::internal::Matcher<VarDecl>, InnerMatcher2) {
159-
const auto *I = Node.body_begin();
160-
if ((I) == Node.body_end())
161-
return false;
162-
163-
return matchTwoVarDecl(
164-
dyn_cast<DeclStmt>(*(I)),
165-
((I + 1) == Node.body_end() ? nullptr : dyn_cast<DeclStmt>(*(I + 1))),
166-
InnerMatcher1, InnerMatcher2, Finder, Builder);
147+
AST_MATCHER_P(CompoundStmt, hasFirstTwoVarDecl,
148+
llvm::SmallVector<ast_matchers::internal::Matcher<VarDecl>>,
149+
InnerMatchers) {
150+
return matchNVarDeclStartingWith(Node.body_begin(), Node.body_end(),
151+
InnerMatchers, Finder, Builder);
167152
}
168153

169154
/// It's not very common to have specifiers for variables used to decompose a
@@ -255,8 +240,10 @@ void UseStructuredBindingCheck::registerMatchers(MatchFinder *Finder) {
255240
hasRHS(expr(hasType(PairType))))
256241
.bind(StdTieAssignStmtName)),
257242
hasPreTwoVarDecl(
258-
varDecl(equalsBoundNode(std::string(FirstVarDeclName))),
259-
varDecl(equalsBoundNode(std::string(SecondVarDeclName))))),
243+
llvm::SmallVector<ast_matchers::internal::Matcher<VarDecl>>{
244+
varDecl(equalsBoundNode(std::string(FirstVarDeclName))),
245+
varDecl(equalsBoundNode(std::string(SecondVarDeclName)))}),
246+
hasParent(compoundStmt().bind(ScopeBlockName))),
260247
this);
261248

262249
// pair<X, Y> p = ...;
@@ -272,7 +259,10 @@ void UseStructuredBindingCheck::registerMatchers(MatchFinder *Finder) {
272259
.bind(PairVarTypeName)),
273260
hasInitializer(expr()))
274261
.bind(PairDeclName)),
275-
hasNextTwoVarDecl(VarInitWithFirstMember, VarInitWithSecondMember)),
262+
hasNextTwoVarDecl(
263+
llvm::SmallVector<ast_matchers::internal::Matcher<VarDecl>>{
264+
VarInitWithFirstMember, VarInitWithSecondMember}),
265+
hasParent(compoundStmt().bind(ScopeBlockName))),
276266
this);
277267

278268
// for (pair<X, Y> p : map) {
@@ -288,9 +278,12 @@ void UseStructuredBindingCheck::registerMatchers(MatchFinder *Finder) {
288278
.bind(PairVarTypeName)),
289279
hasInitializer(expr()))
290280
.bind(PairDeclName)),
291-
hasBody(compoundStmt(hasFirstTwoVarDecl(VarInitWithFirstMember,
292-
VarInitWithSecondMember))
293-
.bind(ScopeBlockName)))
281+
hasBody(
282+
compoundStmt(
283+
hasFirstTwoVarDecl(llvm::SmallVector<
284+
ast_matchers::internal::Matcher<VarDecl>>{
285+
VarInitWithFirstMember, VarInitWithSecondMember}))
286+
.bind(ScopeBlockName)))
294287
.bind(ForRangeStmtName),
295288
this);
296289
}
@@ -320,8 +313,8 @@ void UseStructuredBindingCheck::check(const MatchFinder::MatchResult &Result) {
320313
const auto *FirstVar = Result.Nodes.getNodeAs<VarDecl>(FirstVarDeclName);
321314
const auto *SecondVar = Result.Nodes.getNodeAs<VarDecl>(SecondVarDeclName);
322315

323-
const auto *DS1 = Result.Nodes.getNodeAs<DeclStmt>(FirstDeclStmtName);
324-
const auto *DS2 = Result.Nodes.getNodeAs<DeclStmt>(SecondDeclStmtName);
316+
const auto *BeginDS = Result.Nodes.getNodeAs<DeclStmt>(BeginDeclStmtName);
317+
const auto *EndDS = Result.Nodes.getNodeAs<DeclStmt>(EndDeclStmtName);
325318
const auto *ScopeBlock = Result.Nodes.getNodeAs<CompoundStmt>(ScopeBlockName);
326319

327320
// Captured structured bindings are a C++20 extension
@@ -336,7 +329,7 @@ void UseStructuredBindingCheck::check(const MatchFinder::MatchResult &Result) {
336329
}
337330

338331
const auto *CFRS = Result.Nodes.getNodeAs<CXXForRangeStmt>(ForRangeStmtName);
339-
auto DiagAndFix = [&DS1, &DS2, &FirstVar, &SecondVar, &CFRS,
332+
auto DiagAndFix = [&BeginDS, &EndDS, &FirstVar, &SecondVar, &CFRS,
340333
this](SourceLocation DiagLoc, SourceRange ReplaceRange,
341334
TransferType TT = TT_ByVal) {
342335
StringRef Prefix;
@@ -354,18 +347,15 @@ void UseStructuredBindingCheck::check(const MatchFinder::MatchResult &Result) {
354347
Prefix = "const auto&";
355348
break;
356349
}
357-
std::vector<FixItHint> Hints;
358-
if (DS1)
359-
Hints.emplace_back(FixItHint::CreateRemoval(DS1->getSourceRange()));
360-
if (DS2)
361-
Hints.emplace_back(FixItHint::CreateRemoval(DS2->getSourceRange()));
362350

363351
std::string ReplacementText =
364352
(Twine(Prefix) + " [" + FirstVar->getNameAsString() + ", " +
365353
SecondVar->getNameAsString() + "]" + (CFRS ? " :" : ""))
366354
.str();
367355
diag(DiagLoc, "use structured binding to decompose a pair")
368-
<< FixItHint::CreateReplacement(ReplaceRange, ReplacementText) << Hints;
356+
<< FixItHint::CreateReplacement(ReplaceRange, ReplacementText)
357+
<< FixItHint::CreateRemoval(
358+
SourceRange{BeginDS->getBeginLoc(), EndDS->getEndLoc()});
369359
};
370360

371361
if (const auto *COCE =

0 commit comments

Comments
 (0)