[PATCH] D117613: [ASTMatchers] Add `isConsteval` matcher

[Aaron Ballman via Phabricator via cfe-commits]

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