aaron.ballman added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5179
+/// consteval int bar();
+/// void baz() { if consteval {} }
+/// \endcode
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117613/new/
https://reviews.llvm.org/D117613
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
