sammccall added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2733
+/// \endcode
+AST_MATCHER_P(AttributedStmt, isAttr, attr::Kind, AttrKind) {
+ return llvm::any_of(Node.getAttrs(),
----------------
This definitely seems more like `hasAttr` than `isAttr` to me. An
AttributedStmt *is* the (sugar) statement, and *has* the attribute(s).
Maybe rather `describesAttr` so people don't confuse this for one that walks
up, though?
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2713-2714
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher<Stmt, AttributedStmt>
+ attributedStmt;
+
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > Design-wise, I'm on the fence here. AST matchers match the AST nodes that
> > Clang produces, and from that perspective, this makes a lot of sense. But
> > `AttributedStmt` is a bit of a hack in some ways, and do we want to expose
> > that hack to AST matcher users? e.g., is there a reason we shouldn't make
> > `returnStmt(hasAttr(attr::Likely))` work directly rather than making the
> > user pick their way through the `AttributedStmt`? This is more in line with
> > how you check for a declaration with a particular attribute and seems like
> > the more natural way to surface this.
> >
> > For the moment, since this is following the current AST structure in Clang,
> > I think this is fine. But I'm curious if @klimek or perhaps @sammccall has
> > an opinion here.
> I think a way to find any kind of statement (or expression) that has a
> specific attribute is very useful. How that should look, idk.
TL;DR: I think this matcher fits the current design best.
`returnStmt(hasAttr())` sounds awesome, but I think it's a significant new
concept, and a cross-cutting project like traversal modes.
---
returnStmt(hasAttr()) is definitely nicer in isolation (and in combination with
how decls work).
It's definitely potentially confusing to not match the AST. The AST is unlikely
to be fixed because (IIUC) we don't want to burden each stmt with tracking if
it has attrs.
So I think the easy answer is this patch gets it right.
The inconsistency with decls is annoying, and maybe worse it doesn't yield a
simple way to express "a return statement, maybe with an attribute, I don't
care" unless you're searching recursively anyway, and this should almost be the
default.
`returnStmt(hasAttr())` suggests that it would enable this, maybe by skipping
over the AttributedStmt with some fancy traversal mode, and then looking it up
again in the hasAttr matcher from the parent map.
I think this would be a less brittle way to handle mostly-transparent nodes
that you nevertheless want to be able to match the contents of sometimes. The
current options (explicitly unwrap, rely on recursive search, and traversal
modes) all seem to have significant limitations.
However this is a pretty general idea (and I guess a pretty large project), and
I don't think it's worth introducing just for AttributedStmt.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120949/new/
https://reviews.llvm.org/D120949
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
