[PATCH] D89743: Support Attr in DynTypedNode and ASTMatchers.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D89743#2341900 <https://reviews.llvm.org/D89743#2341900>, @sammccall wrote:

> In D89743#2341779 <https://reviews.llvm.org/D89743#2341779>, @aaron.ballman wrote:
>
>> This is *awesome*, thank you so much for working on it!
>
> Thanks for the comments - haven't addressed them yet, but wanted to clarify scope first.
>
>> One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute"
>
> Yes definitely, I should have mentioned this...
>
> My main goal here was to support it in DynTypedNode, as we have APIs (clangd::SelectionTree) that can only handle nodes that fit there.
> But the common test pattern in ASTTypeTraitsTest used matchers, and I figured a basic matcher wasn't hard to add.

Okay, that makes sense to me.

>> are you planning to extend this functionality so that you can do something like `attr(hasName("foo"))`, or specify the syntax the attribute uses, `isImplicit()`, etc?
>
> I hadn't planned to - it definitely makes sense though I don't have any immediate use cases. I can do any of:
>
> - leave as-is, waiting for someone to add matchers to make it useful
> - scope down the patch to exclude the matcher (and write the ASTTypeTraitsTest another way)
> - add some simple matcher like hasName (I guess in a followup patch) to make it minimally useful, with space for more
>
> What do you think?

My preference is to add support for `hasName()` as that's going to be the most common need for matching attributes. If you also added support for matching which syntax is used, support attributes for `hasArgument()` (which I suppose could be interesting given that there are the parsed attribute arguments and the semantic attribute arguments, and they may differ), etc, I certainly wouldn't complain, but I think at least name matching support is required to make the matcher marginally useful. I'm totally fine if this happens in a follow-up patch (or patches). WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89743/new/

https://reviews.llvm.org/D89743