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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
