[PATCH] D112089: consteval if does not form a discarded statement

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1445-1450
     Sema::ExpressionEvaluationContext Context =
         Sema::ExpressionEvaluationContext::DiscardedStatement;
-    if (NotLocation.isInvalid() && IsConsteval)
+    if (NotLocation.isInvalid() && IsConsteval) {
       Context = Sema::ExpressionEvaluationContext::ImmediateFunctionContext;
+      ShouldEnter = true;
+    }
----------------
rsmith wrote:
> rsmith wrote:
> > It looks to me like this is still incorrectly updating the context in some cases. We shouldn't enter an immediate function context if we're already in a discarded statement context. For example, Clang currently rejects this valid code:
> > 
> > ```
> > auto f() {
> >     if constexpr (false) {
> >         if consteval {
> >             return 0;
> >         }
> >     }
> >     return 0.0;
> > }
> > ```
> > 
> > ... and it looks like it still will after this change. I think we should not enter a new context here if the existing context is a discarded statement context.
> Hm, not updating the context also seems wrong; then we'd reject things like this:
> ```
> consteval int *make() { return new int; }
> auto f() {
>   if constexpr (false) {
>     if consteval {
>       // Immediate function context, so call to `make()` is valid.
>       // Discarded statement context, so `return 0;` is valid too.
>       delete make();
>       return 0;
>     }
>   }
>   return 0.0;
> }
> ```
> Perhaps we need to track whether we're in a discarded statement and whether we're in an immediate function context separately rather than treating them as mutually exclusive. I think it makes sense to go ahead with this patch as-is and deal with the bug that we overwrite a discarded statement context with an immediate function context separately.
Hum, I missed these scenarios entirely. 
But I agree with you, not entering an immediate context would be equally bad.
I think the solution in both cases if to test whether any of the parent context is discarded or immediate, instead of just looking at the current context


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112089/new/

https://reviews.llvm.org/D112089