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