[PATCH] D120949: [clang][AST matchers] adding attributedStmt AST matcher

[Sam McCall via Phabricator via cfe-commits]

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