[PATCH] D74130: [clang] fix consteval call in default arguments

[Tyker via Phabricator via cfe-commits]

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