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

[Alister Johnson via Phabricator via cfe-commits]

ajohnson-uoregon added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2733
+/// \endcode
+AST_MATCHER_P(AttributedStmt, isAttr, attr::Kind, AttrKind) {
+  return llvm::any_of(Node.getAttrs(),
----------------
aaron.ballman wrote:
> sammccall wrote:
> > This definitely seems more like `hasAttr` than `isAttr` to me. An AttributedStmt *is* the (sugar) statement, and *has* the attribute(s).
> > 
> > Maybe rather `describesAttr` so people don't confuse this for one that walks up, though?
> Good point on the `isAttr` name!
> 
> I'm not sure `describeAttr` works (the statement doesn't describe attributes, it... has/contains/uses the attribute). Maybe.. `specifiesAttr()`?
> 
> Or are we making this harder than it needs to be and we should use `hasAttr()` for parity with other things that have attributes associated with them?
Yeah I wasn't sure on the name, I just picked something that wouldn't make me figure out polymorphic matcher declarations because this was the first AST matcher I'd written. :)  

I like `containsAttr()` or `specifiesAttr()`, since it could have multiple attributes. `hasAttr()` makes the most sense to me though. If y'all think we should go with that, the new `hasAttr()` definition would look something like this, right? (To make sure I do in fact understand polymorphic matchers.)


```
AST_POLYMORPHIC_MATCHER_P(
    hasAttr,
    AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, AttributedStmt),
    attr::Kind, AttrKind) {
  return llvm::any_of(Node.getAttrs(),
                      [&](const Attr *A) { return A->getKind() == AttrKind; });
}
```

Original `hasAttr()` for reference:
```
AST_MATCHER_P(Decl, hasAttr, attr::Kind, AttrKind) {
  for (const auto *Attr : Node.attrs()) {
    if (Attr->getKind() == AttrKind)
      return true;
  }
  return false;
}
```


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2713-2714
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher<Stmt, AttributedStmt>
+    attributedStmt;
+
----------------
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.
I think both would be useful. Because sometimes you might want to look for all statements of a particular kind (eg, returnStmts) that have an attributed, and then 'returnStmt(hasAttr())` should work. But there are also cases (like the one I'm working on: https://github.com/ajohnson-uoregon/llvm-project/tree/feature-ajohnson/clang-tools-extra/clang-rewrite) where we want to find all statements with an attribute, but we don't really care what kind of statement it is, in which case looking for AttributedStmts is easier.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120949