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
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.
This doesn't match our coding guidelines. Should be getTraversalKind(), etc. Same below.
Is there a reason we don't want these functions to be marked const?
Let's not make underscores important like this -- how about Node or something generic instead?
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.
Missing a full stop at the end of the comment.
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)?
Please do not use auto here.
Add an assertion message?
I think clang still uses uppercase names everywhere. Can you be more specific?
It looks like they can be const.
Yes, I thought about that, but in a follow-up patch. First, I aim to extend the TraversalKind enum with TK_IgnoreInvisble.
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.
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).
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.
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.
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.
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.
The formatting is wrong here -- be sure to run the patch through clang-format before committing.
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"?
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 :)