sammccall added a comment. In D89743#2356172 <https://reviews.llvm.org/D89743#2356172>, @aaron.ballman wrote:
> I was sort of expecting `hasAttr()` to be deprecated because it is a leaky > abstraction (consumers of the API have to know about our internal naming > convention for attribute kind enumerations, it makes it harder for use to > change the definition of the enumeration, etc. Sure, SGTM >>> I'm totally fine if this happens in a follow-up patch (or patches). WDYT? > > I definitely don't agree that `hasName()` has different semantics for > attributes from declarations -- both of them are named identifiers, > potentially with scope information as part of the name. Making the matcher > polymorphic matches my expectations (pun intended). Fair enough - this isn't really my area, but the differences I was thinking of were: - the Attr uses a name rather than owning it - it's more like DeclRefExpr than like NamedDecl - it wasn't clear to me that you'd want the same rules about optional scope handling, though it sounds like you do >> 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... > > I'm curious if @klimek agrees, but I think `hasName()` is the correct way we > want to eventually go. That said, I think having an attribute matcher with no > way to match by name encourages an anti-pattern where people write a very > generic matcher that matches a lot and then do further refinement work for > every match (rather than taking advantage of memoization and other > optimizations), so I think `hasAttrName()` is still better than nothing. > Would it make sense to add the API but mark it as deprecated to alert users > that this API may go away some day if we can swing it? It's a bit unfriendly > since we don't have a replacement API lined up for users to use, so I > wouldn't want to issue any diagnostics for using the matcher. I've added a "deprecated" comment describing the intent to merge into `hasName`. ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:729 - private: + static bool matchesNode(const NamedDecl &Node, StringRef Name); + ---------------- aaron.ballman wrote: > Are the changes to the `HasNameMatcher` needed? Oops, these were left over from a polymorphism experiment 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
