Tyker added inline comments.
================
Comment at: clang/lib/Sema/Sema.cpp:1026
+ HandleImmediateInvocations(ExprEvalContexts.back());
+
----------------
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.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:17045-17048
+ bool HasConstantInitializer = false;
+ if (auto *VD = dyn_cast<VarDecl>(D))
+ HasConstantInitializer = VD->isConstexpr() || VD->hasAttr<ConstInitAttr>();
+ isConstantEvaluatedOverride = HasConstantInitializer;
----------------
rsmith wrote:
> This needs a comment explaining what's going on: why are `constexpr` /
> `constinit` variables special here, but (for example) a variable of type
> `const int` is not?
>
> Actually, I think this change might not be correct. Consider the following:
>
> ```
> consteval void check(bool b) { if (!b) throw; }
> constinit int x = true ? 1 : (check(false), 2);
> ```
>
> This code is ill-formed: the immediate invocation of `check(false)` is not a
> constant expression. But I think with this code change, we won't require
> `check(false)` to be a constant expression, instead deferring to checking
> whether the initializer of `x` as a whole is a constant expression (which it
> is!). So we'll accept invalid code.
this fixes an issue where we would generate 2 error when we couldn't evaluate a
consteval call inside the initialization of a constexpr/constinit
variable.
```
consteval void check(bool b) { if (!b) throw; }
constinit int x = (check(false), 2);
```
one of those error is "call to consteval function 'check' is not a constant
expression"
the second is "variable does not have a constant initializer"
i fixed it by marking the expression as failed when the consteval call fails.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:17062
ExitDeclaratorContext(S);
+ isConstantEvaluatedOverride = false;
}
----------------
rsmith wrote:
> Do we need to restore the old value here, rather than resetting to `false`?
> In a case like:
>
> ```
> consteval int g() { return 0; }
> constexpr int a = ({ constexpr int b = 0; g(); });
> ```
>
> it looks like we'd lose the 'override' flag at the end of the definition of
> `b`.
>
> Generally I find the idea of using a global override flag like this to be
> dubious: there are a lot of ways in which we switch context during `Sema`,
> and we'd need to make sure we switch out this context at those times too. If
> we can avoid using a global state flag for this (for example by carrying this
> on the `ExpressionEvaluationContextRecord`, that'd be a lot less worrying.
i changed approach and this isn't needed anymore.
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
