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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
