aaron.ballman edited subscribers, added: ymandel; removed: aaron.ballman. aaron.ballman added a comment.
In D89743#2409115 <https://reviews.llvm.org/D89743#2409115>, @sammccall wrote: > In D89743#2409001 <https://reviews.llvm.org/D89743#2409001>, @sammccall wrote: > >> We didn't talk about overloading isImplicit to apply to attrs too, but it >> doesn't seem like that was controversial (and it does help with the tests). > > I spoke too soon about this... > This prevents `hasAncestor(isImplicit())` from compiling, because > `hasAncestor` needs to deduce the node type from its argument to call > `ASTMatchFinder::matchesAncestorOf<T>()`. > This occurs in a few places in tree and many places in our private codebase... > The workaround is `hasAncestor(decl(isImplicit()))` which is reasonable, > except that "is contained in *any* implicit node" is probably actually the > intent. Well, at least it's not a regression. Users can always traverse in `IgnoreUnlessSpelledInSource` mode for that situation though, so at least there's a reasonable path forward. > In addition, while digging into this, I realized Attrs are not traversed in > the parent map, and not supported by the parent/child/ancestor/descendant > traversals. > So I'm fixing that... and adding some tests. Good catch! > I'll need to send this for another round, even without the name matcher. Thank you! ================ Comment at: clang/lib/AST/ASTTypeTraits.cpp:138 + return ASTNodeKind(NKI_##A##Attr); +#include "clang/Basic/AttrList.inc" + } ---------------- 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. ================ 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())))); ---------------- Can you add an expects false test for an unknown attribute and another one for an attribute with no AST node associated with it? 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
