aaron.ballman closed this revision.
aaron.ballman added a comment.
I've commit in ab2ca8496d54573de1c8bec204009567ba2b4086
<https://reviews.llvm.org/rGab2ca8496d54573de1c8bec204009567ba2b4086> and filed
https://bugs.llvm.org/show_bug.cgi?id=52231 as the bug for the remaining bits,
thanks for the reviews and discussion!
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1445-1450
Sema::ExpressionEvaluationContext Context =
Sema::ExpressionEvaluationContext::DiscardedStatement;
- if (NotLocation.isInvalid() && IsConsteval)
+ if (NotLocation.isInvalid() && IsConsteval) {
Context = Sema::ExpressionEvaluationContext::ImmediateFunctionContext;
+ ShouldEnter = true;
+ }
----------------
cor3ntin wrote:
> rsmith wrote:
> > rsmith wrote:
> > > It looks to me like this is still incorrectly updating the context in
> > > some cases. We shouldn't enter an immediate function context if we're
> > > already in a discarded statement context. For example, Clang currently
> > > rejects this valid code:
> > >
> > > ```
> > > auto f() {
> > > if constexpr (false) {
> > > if consteval {
> > > return 0;
> > > }
> > > }
> > > return 0.0;
> > > }
> > > ```
> > >
> > > ... and it looks like it still will after this change. I think we should
> > > not enter a new context here if the existing context is a discarded
> > > statement context.
> > Hm, not updating the context also seems wrong; then we'd reject things like
> > this:
> > ```
> > consteval int *make() { return new int; }
> > auto f() {
> > if constexpr (false) {
> > if consteval {
> > // Immediate function context, so call to `make()` is valid.
> > // Discarded statement context, so `return 0;` is valid too.
> > delete make();
> > return 0;
> > }
> > }
> > return 0.0;
> > }
> > ```
> > Perhaps we need to track whether we're in a discarded statement and whether
> > we're in an immediate function context separately rather than treating them
> > as mutually exclusive. I think it makes sense to go ahead with this patch
> > as-is and deal with the bug that we overwrite a discarded statement context
> > with an immediate function context separately.
> Hum, I missed these scenarios entirely.
> But I agree with you, not entering an immediate context would be equally bad.
> I think the solution in both cases if to test whether any of the parent
> context is discarded or immediate, instead of just looking at the current
> context
> Perhaps we need to track whether we're in a discarded statement and whether
> we're in an immediate function context separately rather than treating them
> as mutually exclusive. I think it makes sense to go ahead with this patch
> as-is and deal with the bug that we overwrite a discarded statement context
> with an immediate function context separately.
I think you may be correct here. I've added your example to the test case with
a FIXME comment so it's clear we've still got some work left to do here.
Thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112089/new/
https://reviews.llvm.org/D112089
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
