This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatchers] Fix hasParent while ignoring unwritten nodes
ClosedPublic

Authored by steveire on Feb 5 2021, 3:19 AM.

Details

Summary
For example, before this patch we can use has() to get from a
cxxRewrittenBinaryOperator to its operand, but hasParent doesn't get
back to the cxxRewrittenBinaryOperator.  This patch fixes that.

Diff Detail

Event Timeline

steveire requested review of this revision.Feb 5 2021, 3:19 AM
steveire created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 3:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steveire retitled this revision from Add test coverage for hasParent() usage to [ASTMatchers] Fix hasParent while ignoring unwritten nodes.Feb 5 2021, 3:21 AM

Can you add a bit more description to the review summary about what's being fixed? It helps the reviewers with context but it also helps when doing code archeology on changes.

clang/lib/AST/ParentMapContext.cpp
138

Why four levels? (May be worth adding that to the comment so it seems like less of a magic number.)

140
143–145
152

If ChildExpr is null, we do a fair amount of work just to get to this point and break out of the while loop -- would it be more clear to add ChildExpr && to the loop predicate?

160–162
174

Not needing to be solved in this patch, but do we eventually need to do something for ObjCForCollectionStmt the same as we do for CXXForRangeStmt?

179–181
185

Not needing to be solved in this patch, but do we need to eventually do something about BlockExpr the same as we do for LambdaExpr?

187–189
195–197
307–309
steveire edited the summary of this revision. (Show Details)Feb 17 2021, 12:48 PM
steveire added inline comments.
clang/lib/AST/ParentMapContext.cpp
174

Perhaps. I'm not familiar with it.

aaron.ballman accepted this revision.Feb 18 2021, 6:45 AM

Update

Thanks for the extra context, that's useful -- LGTM!

clang/lib/AST/ParentMapContext.cpp
174

I'm not super familiar with it myself, but I know it uses for (element in expression) as the syntax which seems awfully similar to for (auto decl : expr) in C++ which is why I thought of it.

This revision is now accepted and ready to land.Feb 18 2021, 6:45 AM