aaron.ballman added a subscriber: sammccall. aaron.ballman added a comment.
In D120949#3358707 <https://reviews.llvm.org/D120949#3358707>, @ajohnson-uoregon wrote: > Okay this is actually the right commits now, sorry about the spam, Aaron. 🙃 > Didn't realize I'd put unrelated things in the first commit. No worries! I usually review AST matcher changes anyway, so there's definitely no harm in tagging me. :-) You also need to add some test coverage for the changes to `clang/unittests/ASTMatchers/` and you also need to regenerate the documentation by running `clang/docs/tools/dump_ast_matchers.py`. ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2704-2712 +/// Example: Matches [[likely]] and [[unlikely]] +/// \code +/// constexpr double pow(double x, long long n) noexcept { +/// if (n > 0) [[likely]] +/// return x * pow(x, n - 1); +/// else [[unlikely]] +/// return 1; ---------------- ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2713-2714 +/// \endcode +extern const internal::VariadicDynCastAllOfMatcher<Stmt, AttributedStmt> + attributedStmt; + ---------------- 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. ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2722 +/// \endcode +/// would only match [[likely]] here: +/// \code ---------------- ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2732-2737 + for (const auto *Attr : Node.getAttrs()) { + if (Attr->getKind() == AttrKind) { + return true; + } + } + return false; ---------------- Probably have to reformat this as well. ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2746 +/// \endcode +/// would match return 1; here: +/// \code ---------------- ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2753-2755 + const Stmt *const Statement = Node.getSubStmt(); + return (Statement != nullptr && + InnerMatcher.matches(*Statement, Finder, Builder)); ---------------- 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
