[PATCH] D80925: Fix compiler crash when an expression parsed in the tentative parsing and must be claimed in the another evaluation context.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:1009
+              Actions, Sema::ExpressionEvaluationContext::Unevaluated,
+              Sema::ReuseLambdaContextDecl);
+          Res = Actions.TransformToPotentiallyEvaluated(Res.get());
----------------
ABataev wrote:
> rjmccall wrote:
> > Pushing an unevaluated context here isn't correct.  Here we're parsing the expression for real and should already be in the right context for `TransformToPotentiallyEvaluated`.
> `TransformToPotentiallyEvaluated` expects that the innermost context is reevaluated. By the time we approach this branch, we're already out of the original unevaluated context. So, we need to create fake unevaluated context to mimic the original one. Function `TransformToPotentiallyEvaluated` copies the context state from the previous context to this new one and rebuilds the expression correctly. Instead, I can add a new function that transforms the expression in the current context and use it in `TransformToPotentiallyEvaluated`
> TransformToPotentiallyEvaluated expects that the innermost context is reevaluated. By the time we approach this branch, we're already out of the original unevaluated context.

Oh, I see.  TransformToPotentiallyEvaluated expects that it will still be in a (temporary) unevaluated context and then looks through that context to build it in the surrounding context.  (In fact, it actually changes the current context to be that outer context and never changes it back.)  So we need to push an unevaluated context to balance things out.

I actually really dislike that design approach; the caller should be expected to have popped off the evaluation context under which the operand was parsed, and we should rebuild in the current context.  There's way too much subtle reasoning about the exact global state here.  But given that it is what it is, please leave a comment explaining why pushing a context is necessary here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80925