Page MenuHomePhabricator

Make it possible control matcher traversal kind with ASTContext
ClosedPublic

Authored by steveire on May 12 2019, 1:25 PM.

Details

Summary

This will eventually allow traversal of an AST while ignoring invisible
AST nodes. Currently it depends on the available enum values for
TraversalKinds. That can be extended to ignore all invisible nodes in
the future.

Diff Detail

Event Timeline

steveire created this revision.May 12 2019, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2019, 1:25 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@klimek This includes a test for the memoization case you were interested in at EuroLLVM.

In this iteration of the API, the traversal kind is changed by using a new traverse matcher, which gives the user control over whether invisible/implicit nodes are skipped or not.

Thanks for this -- it looks like really interesting functionality! I've mostly found nits thus far, but did have a question about clang-query support for it.

include/clang/AST/ASTContext.h
562–563 ↗(On Diff #199185)

This doesn't match our coding guidelines. Should be getTraversalKind(), etc. Same below.

565–568 ↗(On Diff #199185)

Is there a reason we don't want these functions to be marked const?

include/clang/AST/ASTNodeTraverser.h
80 ↗(On Diff #199185)

setTraversalKind()

105 ↗(On Diff #199185)

Let's not make underscores important like this -- how about Node or something generic instead?

113–114 ↗(On Diff #199185)

What an unfortunate name for this. IgnoreParenImpCasts() ignores parens, implicit casts, full expressions, temporary materialization, and non-type template substitutions, and those extra nodes have surprised people in the past.

Not much to be done about it here, just loudly lamenting the existing name of the trait.

include/clang/ASTMatchers/ASTMatchers.h
701 ↗(On Diff #199185)

Missing a full stop at the end of the comment.

716 ↗(On Diff #199185)

Copy pasta?

718 ↗(On Diff #199185)

Is this an API we should be exposing to clang-query as well? Will those users be able to use a string literal for the traversal kind, like they already do for attribute kinds (etc)?

include/clang/ASTMatchers/ASTMatchersInternal.h
286 ↗(On Diff #199185)

traversalKind()

287 ↗(On Diff #199185)

return llvm::None;

lib/AST/ASTContext.cpp
120 ↗(On Diff #199185)

auto *

10358 ↗(On Diff #199185)

Stmt *

lib/ASTMatchers/ASTMatchFinder.cpp
148 ↗(On Diff #199185)

auto *

lib/ASTMatchers/ASTMatchersInternal.cpp
214–215 ↗(On Diff #199185)

Please do not use auto here.

218–219 ↗(On Diff #199185)

Or here...

237–238 ↗(On Diff #199185)

Or here...

240 ↗(On Diff #199185)

Add an assertion message?

steveire updated this revision to Diff 199875.May 16 2019, 11:47 AM
steveire marked 10 inline comments as done.

Update

include/clang/AST/ASTContext.h
562–563 ↗(On Diff #199185)

I think clang still uses uppercase names everywhere. Can you be more specific?

565–568 ↗(On Diff #199185)

It looks like they can be const.

include/clang/ASTMatchers/ASTMatchers.h
718 ↗(On Diff #199185)

Yes, I thought about that, but in a follow-up patch. First, I aim to extend the TraversalKind enum with TK_IgnoreInvisble.

aaron.ballman added inline comments.May 16 2019, 12:00 PM
include/clang/AST/ASTContext.h
562–563 ↗(On Diff #199185)

No, Clang doesn't use uppercase names everywhere -- we're consistently inconsistent and it depends mostly on the age of when the code was introduced and what the surrounding code looks like. We still follow the usual coding style guidelines for naming conventions -- stick with the convention used by nearby code if it's already consistent, otherwise follow the coding style rules.

include/clang/ASTMatchers/ASTMatchers.h
716 ↗(On Diff #199185)

You dropped the interesting bit from the documentation here -- you should add in what the matcher is matching (which makes the preceding "The matcher \code ... \endcode" grammatical again).

718 ↗(On Diff #199185)

This is new functionality, so why do you want to wait for a follow-up patch (is it somehow more involved)? We typically add support for dynamic matchers at the same time we add support for the static matchers because otherwise the two get frustratingly out of sync.

steveire marked 2 inline comments as done.May 16 2019, 12:33 PM
steveire added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
718 ↗(On Diff #199185)

Perhaps I can add the support in DynamicASTMatchers. Can you give some direction on that? I don't recall where the attr strings are handled.

A corresponding change to clang-query will be in another patch anyway because it's a different repo.

aaron.ballman added inline comments.May 16 2019, 12:52 PM
include/clang/ASTMatchers/ASTMatchers.h
718 ↗(On Diff #199185)

Perhaps I can add the support in DynamicASTMatchers. Can you give some direction on that? I don't recall where the attr strings are handled.

I always wind up having to use svn blame to figure it out, so you're not alone in your recollections. ;-) You add a new specialization of ArgTypeTraits to Marshallers.h for the enumeration type. See around line 123 or so for how it's done for attr::Kind.

A corresponding change to clang-query will be in another patch anyway because it's a different repo.

Once you have the enumeration being marshaled, I believe all you have to do is expose the new API via Registry.cpp like any other matcher, though I'd also request tests be added to Dynamic/RegistryTest.cpp since this is adding a small amount of new marshaling code.

steveire marked an inline comment as done.May 17 2019, 5:37 AM
steveire added inline comments.
lib/ASTMatchers/ASTMatchersInternal.cpp
240 ↗(On Diff #199185)

Saying what? The original code doesn't have one. Let's avoid round trips in review comments :).

aaron.ballman added inline comments.May 17 2019, 6:23 AM
lib/AST/ASTContext.cpp
120 ↗(On Diff #199185)

The formatting is wrong here -- be sure to run the patch through clang-format before committing.

lib/ASTMatchers/ASTMatchersInternal.cpp
240 ↗(On Diff #199185)

This isn't a "round trip"; it's not unreasonable to ask people to NFC improve the code they're touching (it's akin to saying "Because you figured out this complex piece of code does X, can you add a comment to it so others don't have to do that work next time.").

As best I can tell, this assertion exists because this function is meant to mirror matches() without this base check in release mode. You've lost that mirroring with your refactoring, which looks suspicious. Is there a reason this function deviates from matches() now?

As for the assertion message itself, how about "matched a node of an unexpected derived kind"?

steveire marked an inline comment as done.May 17 2019, 7:44 AM
steveire added inline comments.
lib/ASTMatchers/ASTMatchersInternal.cpp
240 ↗(On Diff #199185)

Hmm, maybe that was a misunderstanding. Your note about an assertion message was not clear, so I asked you what you suggest the message should be. No need for offense :)

steveire updated this revision to Diff 230678.Nov 22 2019, 9:53 AM

Rebase and update

aaron.ballman accepted this revision.Nov 23 2019, 12:44 PM

LGTM with a few minor nits.

clang/include/clang/AST/ASTContext.h
3009

Do we need to use std::map here, or could we use an IndexedMap or DenseMap?

(std::map tends to perform poorly under some circumstances and the keys in this case look like they should be small and dense, but if we can't use one of the more specialized containers for some reason, it's fine to stick with std::map.)

clang/lib/AST/ASTContext.cpp
121

Because N is const, does its get<> method return a const pointer? If so, switch to const auto * for clarity.

clang/lib/ASTMatchers/ASTMatchersInternal.cpp
222–228

raii -> RAII per naming conventions.

This revision is now accepted and ready to land.Nov 23 2019, 12:44 PM
This revision was automatically updated to reflect the committed changes.

I tried using DenseMap, but couldn't make it compile. I stuck with std::map for now.

ymandel added inline comments.
include/clang/ASTMatchers/ASTMatchersInternal.h
286 ↗(On Diff #199185)

Stephen -- What was the resolution on this comment? I came across this when writing my recent fixes and it stood out for going against the style guide and the surrounding style. Is this intentional or an oversight?

Thanks!