Page MenuHomePhabricator

Make it possible control matcher traversal kind with ASTContext
Needs ReviewPublic

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

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

565–568

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

include/clang/AST/ASTNodeTraverser.h
80

setTraversalKind()

105

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

113–114

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

Missing a full stop at the end of the comment.

716

Copy pasta?

718

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

traversalKind()

287

return llvm::None;

lib/AST/ASTContext.cpp
120

auto *

10353

Stmt *

lib/ASTMatchers/ASTMatchFinder.cpp
148

auto *

lib/ASTMatchers/ASTMatchersInternal.cpp
214–224

Please do not use auto here.

218–219

Or here...

237–242

Or here...

240

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

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

565–568

It looks like they can be const.

include/clang/ASTMatchers/ASTMatchers.h
718

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

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

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

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

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

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

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

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

lib/ASTMatchers/ASTMatchersInternal.cpp
240

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

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 :)