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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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(),
----------------
ajohnson-uoregon wrote:
> 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;
> }
> ```
> I like `containsAttr()` or `specifiesAttr()`, since it could have multiple attributes. 

The same is true for declarations; they can have multiple attributes. Which is an interesting test case for us to add for statements:
```
int foo();

int main() {
  [[clang::nomerge]] [[likely]] return foo();
}
```
We should verify we can find both of those attributes on the same `attributedStmt()` (e.g., you don't have to do something like `attributedStmt(attributedStmt(...))` to match.)

As for the name, I think `hasAttr()` is fine; if it causes awkwardness in the future when we change the AST in Clang, we'll have to fix it up then, but that awkwardness will already be there for `attributedStmt()` itself.

> 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?

That looks about like what I'd expect, yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120949