[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think this LGTM, but I'm holding off on signing off until @shafik is satisfied with the part he was asking about. You should add a release note for the fix, too.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:17891-17894
+      // It is possible that some subexpression of current immediate invocation
+      // was handled from another expression evaluation context. Do not handle
+      // current immediate invocation if some of its subexpressions failed
+      // before.
----------------
Minor grammar nits.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17973
 
-  /// When we have more then 1 ImmediateInvocationCandidates we need to check
-  /// for nested ImmediateInvocationCandidates. when we have only 1 we only
-  /// need to remove ReferenceToConsteval in the immediate invocation.
-  if (Rec.ImmediateInvocationCandidates.size() > 1) {
+  /// When we have more then 1 ImmediateInvocationCandidates or previously
+  /// failed immediate invocations, we need to check for nested
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:17979
+  if (Rec.ImmediateInvocationCandidates.size() > 1 ||
+      SemaRef.FailedImmediateInvocations.size()) {
 
----------------
Fznamznon wrote:
> cor3ntin wrote:
> > Shouln't we clear `FailedImmediateInvocations` at the end of a full expression to avoid going there if there was an error in another expression
> The idea sounds reasonable, but I'm not sure it can work with current algorithm of handling immediate invocations. Immediate invocations are handled when expression evaluation context is popped, so we need to remember previously failed immediate invocations until then, even though they could happen in two different full expressions. 
> 
> ```
> consteval foo() { ... }
> void bar() {
>   int a = foo(/* there might be a failed immediate invocation attached to initializer context */); // this is a full-expression
>   int b = foo(); // this is another full-expression
> } // This is where immediate invocations that appear in function context are processed
> ```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234