aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM with a minor adjustment to the documentation.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5173
+/// Matches consteval function declarations and if consteval.
+///
----------------
Needs to be re-wrapped to 80-col, mostly wanting to call out the `if !
consteval` bit more explicitly.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5179
+/// consteval int bar();
+/// void baz() { if consteval {} }
+/// \endcode
----------------
aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > Izaron wrote:
> > > > > aaron.ballman wrote:
> > > > > > It'd be good to show an example of:
> > > > > > ```
> > > > > > if ! consteval {}
> > > > > >
> > > > > > if ! consteval {} else {}
> > > > > > ```
> > > > > > as well so users know what to expect.
> > > > > >
> > > > > > Should there be a matcher so users can distinguish between `if
> > > > > > consteval` and `if ! consteval`?
> > > > > >
> > > > > > Test cases for these sort of things would also be appreciated.
> > > > > Thanks!
> > > > > > Should there be a matcher so users can distinguish between `if
> > > > > > consteval` and `if ! consteval`?
> > > > > This is an interesting question. I wonder how intensively `if !
> > > > > consteval` is ever going to be used in real life to make a unique
> > > > > matcher for it? I can't make up a hypothetical checker that would
> > > > > need it.
> > > > >
> > > > > Let's see what other fellow reviewers think about it =)
> > > > That's a good question.
> > > > Maybe we can add it later if there is a need?
> > > > I suspect that if we need 2, we need 3, `hasConsteval`,
> > > > `hasNegatedConsteval`, `hasNonNegatedConsteval`
> > > >
> > > > Until a need present itself, maybe one is enough
> > > > This is an interesting question. I wonder how intensively if !
> > > > consteval is ever going to be used in real life to make a unique
> > > > matcher for it? I can't make up a hypothetical checker that would need
> > > > it.
> > >
> > > That's not the salient question. I'm more worried that the current
> > > interface is confusing and not easy to extend. There's no motivation in
> > > the review for why this support is needed, and we tend to err on the side
> > > of being conservative with matcher interfaces (we only add them when
> > > they're necessary and generally useful, otherwise users can use local
> > > matchers).
> > >
> > > I think the matcher isn't well designed for `IfStmt` and we should drop
> > > support for that initially, and only focus on support for `FunctionDecl`
> > > where the behavior is more clear. Then we can decide what the correct
> > > interface is for `IfStmt` once we have a better picture of how the
> > > matcher is expected to be used and a more general design for it.
> > That's not the salient question. I'm more worried that the current
> > interface is confusing and not easy to extend. There's no motivation in the
> > review for why this support is needed, and we tend to err on the side of
> > being conservative with matcher interfaces (we only add them when they're
> > necessary and generally useful, otherwise users can use local matchers).
> >
> > Arguably, there is one for `if constexpr` and both of them should be
> > symmetric. Introducing asymmetry for no good reason seems wrong. I assume
> > that there was a motivation initially?
> >
> > > I think the matcher isn't well designed for `IfStmt` and we should drop
> > > support for that initially, and only focus on support for `FunctionDecl`
> > > where the behavior is more clear. Then we can decide what the correct
> > > interface is for `IfStmt` once we have a better picture of how the
> > > matcher is expected to be used and a more general design for it.
> >
> >
> > This was initially my gut feeling, however there is no separate matcher for
> > `if constexpr(false)` and `if constexpr (true)`, so I don't think that `if
> > ! consteval` needs to be treated differently here.
> > Either way, i don't think adding one hurts
> >
> >
> >
> >
> You can match over the expression in `if constexpr` to determine whether it's
> true or false (at least in theory, we probably need some better matchers
> around constant expression value comparisons), but you cannot match over the
> syntax of `if ! consteval` vs `if consteval`, so I see them as being
> different cases in that regard.
I spoke with @cor3ntin off-list a bit about this and am now more comfortable.
What was bothering me was that:
```
if consteval {
}
if ! consteval {
}
```
are both matched by `ifStmt(isConsteval())` and matching on the second case
looks downright odd to me. However, that is the interface of our AST. We can
add `isNonNegatedConsteval()` and `isNegatedConsteval()` as matchers when we
need to distinguish between the cases. Thanks for bearing with me while I
figured that out. :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117613/new/
https://reviews.llvm.org/D117613
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
