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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith 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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112089