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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

I've commit in ab2ca8496d54573de1c8bec204009567ba2b4086 <https://reviews.llvm.org/rGab2ca8496d54573de1c8bec204009567ba2b4086> and filed https://bugs.llvm.org/show_bug.cgi?id=52231 as the bug for the remaining bits, thanks for the reviews and discussion!



================
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;
+    }
----------------
cor3ntin wrote:
> 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
> 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.

I think you may be correct here. I've added your example to the test case with a FIXME comment so it's clear we've still got some work left to do here.  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112089