sammccall added inline comments.
================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2713-2714 +/// \endcode +extern const internal::VariadicDynCastAllOfMatcher<Stmt, AttributedStmt> + attributedStmt; + ---------------- 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). 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
