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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
