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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2713-2714
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher<Stmt, AttributedStmt>
+    attributedStmt;
+
----------------
sammccall wrote:
> aaron.ballman wrote:
> > sammccall wrote:
> > > 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.
> > Thanks for the feedback Sam!
> > 
> > > The AST is unlikely to be fixed because (IIUC) we don't want to burden each stmt with tracking if it has attrs.
> > 
> > I'm less convinced of this. We didn't want to do it originally because there were so very few statement attributes. These days, there's quite a few more more statement attributes, so we may very well revisit this. `AttributedStmt` is a pain that has caused us problems in the past with things like `isa<FooStmt>()` failing because it didn't expect an attributed `FooStmt`.
> > 
> > That said, the rest of your points are compelling, so this matcher is fine for me.
> > We didn't want to do it originally because there were so very few statement attributes.
> 
> Ah, i assumed it was some kind of issue with size or the logistics of allocation. Fixing the AST sounds good then! (But I wouldn't block this patch on it unless someone's ready to work on it).
Yeah, I'm not ready to work on it and I don't know of anyone else planning to do that work any time soon (it's more of an idle "someday" task than anything we need to do immediately). So definitely agreed, let's not block this patch on it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120949