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

[Sam McCall via Phabricator via cfe-commits]

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