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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> (while this is useful to you, let's keep discussing, but I'm also happy to stop and land this with your preferred API/semantics - just LMK if changes are needed)

Let's hold off on landing for just a little bit. I don't think changes are needed yet, but the discussion may change my opinion and we might as well avoid churn. However, if you need to land part of this while we discuss other parts, LMK.

> In D89743#2360032 <https://reviews.llvm.org/D89743#2360032>, @aaron.ballman wrote:
>
>> 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`.
>
> My mental model is that the Attr **classes** own the names, and the Attr **instances** refer to the classes using the names.
>
> So the class `AlignedAttr` has the name `aligned`, and the instance `[[gnu::aligned(8)]]` refers to it. Just like the `FunctionDecl` `double sqrt(double);` has the name `sqrt`, and the DeclRefExpr in `sqrt(8)` refers to it.
>
> To reinforce this, I can't choose a different name for the instance: `[[gnu::whatever]]` doesn't produce an Attr instance at all. But I can choose a different name for the class (by modifying Attr.td).
> So an Attr instance (what we're matching) feels more like a reference than a primary entity. (Similarly, the **semantics** are associated with the Attr class rather than the instance).
>
> If clang's AST modeled syntax rather than semantics, I imagine there'd be one `Attr` class and each instance could have an arbitrary name and arg list. In that case an Attr would seem to "have" a name in the same sense a decl does.

Oh, how interesting, thank you for expounding!

My mental model is that there is no declaration of an attribute (in the usual sense, because users cannot specify their own attributes without changing the compiler), and so there's not a referential model like there are with a DeclRefExpr that refers back to a specific declaration. Instead, to me, an attribute is a bit more like a builtin type --  you may have multiple ways to spell the type (`char` vs `signed char` or `unsigned char`, `long int` vs `long`, etc), but it's the same type under the hood.

However, that brings up an interesting observation: you can't do `hasType(hasName("long"))` and instead have to do `hasType(asString("long"))`. I don't recall the background about why there is `asString()` for matching string types vs `hasName()` -- you wouldn't happen to remember that context, would you? For instance, is the correct pattern to allow `attr(asString("aligned"))` to map back to an `AlignedAttr` regardless of how the attribute was spelled in source, similar to how `asString("long")` matches `long int`?

>> 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.
>
> That definitely seems like the common case. However I can certainly imagine wanting to e.g. write a migration from `__attribute__((xxx))` to `[[gnu::xxx]]`.

Definitely true! I was imagining we'd want a separate checker for narrowing based on attribute syntax.

> (If attr **classes** were AST nodes too, like DeclRefExpr/Decl or TypeLoc/Type then you'd resolve this by using a matcher on one or the other)
>
> As it stands it seems too surprising that `Attr::getAttrName()` returns the particular name used for that instance, but `attr(hasAttrName("..."))` would match any name associated with the same class.
> Maybe `attr(equivalentAttrName("..."))` or so, possibly constrasting to `attr(exactAttrName("..."))`?

Yeah, I am starting to see this more readily, so thank you for continuing the discussion!

>> 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.
>
> Yup, I don't have a real opinion here, I just assumed it was different. Seems like it would work fine.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89743