[PATCH] D147531: [clang] Reset track of immediate function context when entering new function

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon marked 2 inline comments as done.
Fznamznon added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:15186
+  // different handling.
+  ExprEvalContexts.back().InImmediateFunctionContext = FD->isConsteval();
+
----------------
cor3ntin wrote:
> shafik wrote:
> > I am a bit concerned that we are updating `InImmediateFunctionContext` outside of `PushExpressionEvaluationContext`. So we are now spreading the logic outside of where it was originally contained.
> > 
> > I am wondering if instead inside of `PushExpressionEvaluationContext` we could set `InImmediateFunctionContext` like so:
> > 
> > ```
> > ExprEvalContexts.back().InImmediateFunctionContext =
> >       ExprEvalContexts[ExprEvalContexts.size() - 2]
> >           .isImmediateFunctionContext() || NewContext == ImmediateFunctionContext;
> > ```
> > 
> > or something along those lines.
> Here `isImmediateFunctionContext()` will be true, we want it to be false
> 
> If we wanted to do somelink like what you are suggesting, we'd need something like a new `EnteringFunctionDefinitionContext` boolean parameter to  PushExpressionEvaluationContext. so that
> InImmediateFunctionContext is only set to the outer context value when `EnteringFunctionDefinitionContext`would be false
> 
> Given that we should only get in that situation from `ActOnStartOfFunctionDef`, I'm not sure that would be a better design
Thanks, @cor3ntin .
This is the exact reason why I implemented the fix this way.


================
Comment at: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp:120
+
+namespace GH61142 {
+
----------------
aaron.ballman wrote:
> Would it make sense to add some CHECK lines to check the actual codegen is sensible?
Well, the assertion was due to consteval function reaching codegen, so I added checks that it doesn't appear in the IR in any way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147531