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

Tue Oct 27 05:41:37 PDT 2020

aaron.ballman added a subscriber: klimek.
aaron.ballman added a comment.

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

> In D89743#2342229 <https://reviews.llvm.org/D89743#2342229>, @aaron.ballman wrote:
>
>> My preference is to add support for `hasName()` as that's going to be the most common need for matching attributes.
>
> Sounds good (though I ran into issues calling this `hasName` specifically, see below). There's overlap with hasAttr(Kind) but I think that only handles decls, you can't bind the attribute itself, and it's harder to extend to the other stuff you mentioned.
> So maybe hasAttr gets deprecated, or maybe `has(attr(hasName("foo")))` is a mouthful for a common case so the shorthand is nice.

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.

>> If you also added support for matching which syntax is used, support attributes for `hasArgument()` (which I suppose could be interesting given that there are the parsed attribute arguments and the semantic attribute arguments, and they may differ), etc, I certainly wouldn't complain, but I think at least name matching support is required to make the matcher marginally useful.
>
> Yeah, in the interests of time I think i'll need to stop there (my understanding of the attribute model is pretty superficial).

That's fine by me, thank you for the bits you've done!

>> I'm totally fine if this happens in a follow-up patch (or patches). WDYT?
>
> For `hasAttrName` and `isImplicit` (which simplifies the tests), I've tried to fold it into this patch, it's simple enough.
> I went with `hasAttrName` (matching `Attr::getAttrName()`) because I wasn't easily able to get `attrName` to work.
>
> The issue is that `hasName` is already the name of a Decl matcher with the same signature, so we'd have to make it polymorphic.
> (This means they'd appear to be one matcher, with one set of docs etc, despite having very different semantics - doesn't really seem right...)

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

> The implementation of the Decl version is the fairly complex HasNameMatcher (shared with `hasAnyName`) that copies strings into a vector on construction. This doesn't fit into the polymorphic matcher model as far as I can find, so we're adding heap allocations to the fastpath of matching hasName("foo") which seems... not obviously a good tradeoff.

This, on the other hand, is a very good reason to not push forward with those changes yet.

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

================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6764
+AST_MATCHER_P(Attr, hasAttrName, llvm::StringRef, Name) {
+  return Node.getAttrName() && Node.getAttrName()->isStr(Name);
+}
----------------
If you want to avoid duplicating the call to `getAttrName()` (I don't insist).
```
if (const IdentifierInfo *II = Node.getAttrName())
  return II->isStr(Name);
return false;
```

================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:729

- private:
+  static bool matchesNode(const NamedDecl &Node, StringRef Name);
+
----------------
Are the changes to the `HasNameMatcher` needed?

================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp:1893
+                      attr(hasAttrName("nonnull"))));
+  // On windows, some nodes an implicit visibility attribute.
+  EXPECT_TRUE(
----------------
nodes an -> nodes create an

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89743