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

[Aaron Ballman via Phabricator via cfe-commits]

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