[PATCH] D76447: Apply ConstantEvaluated evaluation contexts to more manifestly constant evaluated scopes

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16793-16797
+  if (isManifestlyEvaluatedVar(*this, D)) {
+    using ExpressionKind = ExpressionEvaluationContextRecord::ExpressionKind;
+
+    PushExpressionEvaluationContext(
+        ExpressionEvaluationContext::ConstantEvaluated, D, ExpressionKind::EK_ConstexprVarInit);
----------------
rsmith wrote:
> We can't implement the checks for manifestly constant-evaluated initializers this way in general. Per [expr.const]/14, we need to treat the initializer as manifestly constant-evaluated if it's "the initializer of a variable that is usable in constant expressions or has constant initialization". We can't test either of those conditions in general until we know what the initializer is, because they can both depend on whether evaluation of the initializer succeeds. (This approach works for the case of `constexpr` variables, which are always usable in constant expressions, but not any of the other cases, and the approach we'll need for the other cases will also handle `constexpr` variables. There is a very modest benefit to special-casing `constexpr` variable initializers regardless -- we can avoid forming and then pruning out nested `ConstantExpr` nodes for immediate invocations inside the initializer -- but I think it's probably not worth the added complexity.)
> 
> To address the general problem, we should handle this in `CheckCompleteVariableDeclaration`, which is where we evaluate the initializer and generally determine whether the variable has constant initialization and / or is usable in constant expressions. Probably the cleanest approach -- and certainly the one I'd been intending to pursue -- would be to wrap the initializer with a `ConstantExpr` there in the relevant cases, and allow the usual handling of nested immediate invocations to prune out any `ConstantExpr`s nested within the initializer representing inner calls to `consteval` functions. (I think I've mentioned elsewhere that I would like to remove the "evaluated value" storage on `VarDecl` in favor of using `ConstantExpr` for this purpose.)
> There is a very modest benefit to special-casing constexpr variable initializers regardless -- we can avoid forming and then pruning out nested ConstantExpr nodes for immediate invocations inside the initializer -- but I think it's probably not worth the added complexity.

So, this patch is motivated for us by the desire to check if a "meta type" variable, belongs to a compile time, or runtime. Additionally, this is used being used to verify our compile time, "injection statement", is not appearing in a runtime context. This prevents reflections/metaprogramming values and logic from leaking nonsensically into runtime code. 

I think this can be accomplished as you said in the `CheckCompleteVariableDeclaration`. We're already checking for out of place "meta type" variables there, though we catch fragments, and injection statements more eagerly. In retrospect, I don't think there is any issue removing the eager check from the fragments, and I don't think the checking of the injection statements should be affected by this case.

I'll try and look more into this in the morning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76447