rsmith added inline comments.
================
Comment at: clang/lib/Sema/Sema.cpp:1026
+ HandleImmediateInvocations(ExprEvalContexts.back());
+
----------------
Tyker wrote:
> rsmith wrote:
> > What do we need this for? If I'm understanding the patch correctly, I think
> > the only way we should propagate immediate invocations upwards should be
> > from the `ExpressionEvaluationContext` of a lambda default argument into
> > the `ExpressionEvaluationContext` of the lambda itself, so it should never
> > reach the top level.
> This is fixing a separate bug that consteval call in to top-level
> ExprEvalContext are not handled.
> like this:
> ```
> struct S {
> consteval S(bool b) {if (b) throw;}
> };
> S s2(true);
> ```
> this emits no error.
> because the immediate invocation are added to global scope and the global
> scope is never poped via PopExpressionEvaluationContext.
In this case, what's supposed to happen is that we create an
`ExpressionEvaluationContext` for the initializer of `s2`. See
`Sema::ActOnCXXEnterDeclInitializer` / `Sema::ActOnCXXExitDeclInitializer`. I
guess that's not working somehow?
Ah, the problem is that we leave the context too early in
`Parser::ParseDeclarationAfterDeclaratorAndAttributes`. We call
`InitScope.pop()` before we add the initializer to the declaration, which is
where we actually finish building the initialization full-expression -- that
seems wrong in general. (We're also missing an `InitializerScopeRAII` object
around the call to `ActOnUninitializedDecl` in the case where there is no
initializer -- we need an `ExpressionEvaluationContext` for the
default-initialization in that case too.)
================
Comment at: clang/lib/Sema/SemaExpr.cpp:16232
SemaRef.Diag(Note.first, Note.second);
+ CE->markFailed();
return;
----------------
Changing the dependence flags on an expression isn't safe to do this late: the
`Error` dependence bit should be propagated to all transitive enclosing AST
nodes when they're created, which has already happened at this point. If we
need a way to represent an expression that's supposed to be a constant
expression but isn't constant, perhaps adding a separate flag to `ConstantExpr`
would be reasonable.
================
Comment at: clang/lib/Sema/SemaLambda.cpp:897-901
+ ExprEvalContexts.back().ReferenceToConsteval.insert(
+ LSI->ReferenceToConsteval.begin(), LSI->ReferenceToConsteval.end());
+ ExprEvalContexts.back().ImmediateInvocationCandidates.append(
+ LSI->ImmediateInvocationCandidates.begin(),
+ LSI->ImmediateInvocationCandidates.end());
----------------
Thanks! I think this can be cleaned up a little bit further:
* Move the handling of lambda parameter scopes from
`HandleImmediateInvocations` to `PopExpressionEvaluationContext`
* Change `HandleImmediateInvocations` to take the `ReferenceToConsteval` and
`ImmediateInvocationCandidates` data instead of an
`ExpressionEvaluationContextRecord`
* Maybe consider wrapping `ReferenceToConsteval` and
`ImmediateInvocationCandidates` in a struct to make it a bit easier to pass
them around as a set.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74130/new/
https://reviews.llvm.org/D74130
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
