sammccall added a comment. In D89743#2342229 <https://reviews.llvm.org/D89743#2342229>, @aaron.ballman wrote:
> 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. Sounds good (though I ran into issues calling this `hasName` specifically, see below). There's overlap with hasAttr(Kind) but I think that only handles decls, you can't bind the attribute itself, and it's harder to extend to the other stuff you mentioned. So maybe hasAttr gets deprecated, or maybe `has(attr(hasName("foo")))` is a mouthful for a common case so the shorthand is nice. > 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. Yeah, in the interests of time I think i'll need to stop there (my understanding of the attribute model is pretty superficial). > I'm totally fine if this happens in a follow-up patch (or patches). WDYT? For `hasAttrName` and `isImplicit` (which simplifies the tests), I've tried to fold it into this patch, it's simple enough. I went with `hasAttrName` (matching `Attr::getAttrName()`) because I wasn't easily able to get `attrName` to work. The issue is that `hasName` is already the name of a Decl matcher with the same signature, so we'd have to make it polymorphic. (This means they'd appear to be one matcher, with one set of docs etc, despite having very different semantics - doesn't really seem right...) The implementation of the Decl version is the fairly complex HasNameMatcher (shared with `hasAnyName`) that copies strings into a vector on construction. This doesn't fit into the polymorphic matcher model as far as I can find, so we're adding heap allocations to the fastpath of matching hasName("foo") which seems... not obviously a good tradeoff. If you think the `hasName` name is important I'm happy to drop `hasAttrName` from this patch, and someone can work out how to add it with the better name, but I'm not sure I want to pick this battle with the template gods... 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
