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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from a minor testing request.



================
Comment at: clang/lib/AST/ASTTypeTraits.cpp:138
+    return ASTNodeKind(NKI_##A##Attr);
+#include "clang/Basic/AttrList.inc"
+  }
----------------
ymandel wrote:
> aaron.ballman wrote:
> > Oye, this brings up an interesting point. Plugin-based attributes currently cannot create their own semantic attribute, but will often instead reuse an existing semantic attribute like `annotate`. This means code like `[[clang::plugin_attr]] int x;` may or may not be possible to match. Further, some builtin attributes have no semantic attribute associated with them whatsoever: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/Attr.td#L2740
> > 
> > I think the `switch` statement logic here is correct in these weird cases and we won't hit the `llvm_unreachable`. For attributes with no AST representation, there's no `Attr` object that could be passed in the first place. Unknown attributes similarly won't get here because there's no way to get an AST node for them. Plugin-based attributes are still going to be similarly surprising, but... I don't know that we can solve that here given there's no way to create a plugin-based semantic attribute yet.
> > 
> > Pining @ymandel to raise awareness of these sorts of issues that stencil may run into. For the AST matchers, I think it's reasonable for us to say "if there's no AST node, we can't match on it", but IIRC, stencil was looking to stay a bit closer to the user's source code rather than be strongly tied to the AST.
> @aaron.ballman Thanks for pinging me. However, `Stencil` is limited to AST nodes, for better or worse. They make it somewhat easier to _generate_ source plainly, but they are fundamentaly an abstraction over the AST.  I think that the only way we'll get beyond the AST is Syntax Trees.
> 
> Still, nice to see this patch! I've been meaning to do the same for LambaCapture for a while and this will be a handy guide to what needs to be changed.
That's good to know, @ymandel, thank you!


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp:1887
+  // On windows, some nodes have an implicit visibility attribute.
+  EXPECT_TRUE(
+      notMatches("struct F{}; int x(int *);", attr(unless(isImplicit()))));
----------------
aaron.ballman wrote:
> Can you add an expects false test for an unknown attribute and another one for an attribute with no AST node associated with it?
This comment may have been missed during the discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89743