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

Wed Oct 28 12:39:28 PDT 2020

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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

> 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

Thank you for giving this feedback, it's helpful! I may have to rethink this a bit because it never dawned on me that someone would think of attributes being more like a `DeclRefExpr` than a `NamedDecl`. If I'm following along properly, this is because the AST node for the attribute could have been generated from various ways of spelling it? e.g., `gcc::aligned` vs `_Alignas` vs `align` vs `alignas` all making an `AlignedAttr`. I was thinking this is more like `NamedDecl` because *one* of those names generated the AST node and I'd hate for AST matchers to have to do something like `anyOf(attr(hasSpelling("GNU"), hasName("aligned")), attr(hasSpelling("CXX"), hasName("gcc::aligned)), attr(hasSpelling("declspec"), hasName("align")), attr(hasSpelling("keyword"), hasAnyName("_Alignas", "alignas")))` to match an `AlignedAttr` regardless of how it is spelled. I figured it would make more sense for `attr(hasName("aligned"))` to match any `AlignedAttr` regardless of how the user wrote it in their source code, but perhaps others have different ideas here.

The scope handling is a bit of a question mark. Attributes don't really have namespaces like other identifiers, but they sort of have them when you squint at the name just right. For instance, the scope handling currently allows you to match `::foo` in C mode even though C doesn't have the notion of namespaces. So I thought it wouldn't be unintuitive for it to also work for namespace scopes as well.

>>> 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`.

Thanks! LGTM! Please wait a day or two before landing for @klimek to respond if he has any concerns about introducing a new matcher that's already deprecated.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89743