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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(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)

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.

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

(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("..."))`?

> 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