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