[PATCH] D143053: [C++20] [Modules] Pop Expression Evaluation Context when we skip its body during parsing

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin accepted this revision.
cor3ntin added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Parse/Parser.cpp:1415
+    //
+    // FIXME: It looks not easy to balance PushExpressionEvaluationContext()
+    // and PopExpressionEvaluationContext().
----------------
ChuanqiXu wrote:
> cor3ntin wrote:
> > shafik wrote:
> > > It does seem a bit ad-hoc
> > could we use `ExitFunctionBodyRAII` here, maybe ? It already deals with lambdas
> > It does seem a bit ad-hoc
> 
> I agree. I feel the current explicit style for pushing and popping different contexts looks not easy to maintain... I just want to prevent the crash for now and I feel the patch wouldn't be a burden when someday we want to refactor this.
> 
> > could we use ExitFunctionBodyRAII here, maybe ? It already deals with lambdas
> 
> Maybe. But it wouldn't be simpler. We need to move `ExitFunctionBodyRAII` and we need to write:
> 
> ```
> ExitFunctionBodyRAII ExitRAII(Actions, isLambdaCallOperator(dyn_cast_if_present<FunctionDecl>(Res)));
> return Res;
> ```
> 
> which looks odd and unnecessary.
> 
> And if you're saying something like:
> 
> ```
> Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D,
>                                               TemplateInfo.TemplateParams
>                                                   ? *TemplateInfo.TemplateParams
>                                                   : MultiTemplateParamsArg(),
>                                               &SkipBody, BodyKind);
> 
> 
> ExitFunctionBodyRAII ExitRAII(Actions, isLambdaCallOperator(dyn_cast_if_present<FunctionDecl>(Res)));
> ```
> 
> and remove the ExitFunctionBodyRAII in `ActOnFinishFunctionBody`. I guess it can't work in this way simply. Since the comment (https://github.com/llvm/llvm-project/blob/c6795b1d37cee586d9b98dade64432f8f6bd004b/clang/lib/Sema/SemaDecl.cpp#L15811-L15818) says we need to pop expression evaluation context before popping decl context (and function scope info?).
I think you are right, your change is probably as good as anything without doing a major refactor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143053