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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Parse/Parser.h:2928
+      SmallVectorImpl<DeclaratorChunk::ParamInfo> &ParamInfo,
+      SourceLocation &EllipsisLoc, bool InConstantConstext = false);
   void ParseBracketDeclarator(Declarator &D);
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:1145
+    enum ImmediateInvocationEndScopeAction {
+      IIESA_Handle, ///< Immediate invocation can and should be handeled at the
+                    ///< end of the scope.
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:1147
+                    ///< end of the scope.
+      IIESA_Propagate, ///< Immediate invocation cannot be handeled at the end
+                       ///< of the scope and should be propagated to the outer
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:4690
+          ExpressionEvaluationContextRecord::EK_Other,
+      bool IsLambdaParamScope = false);
   enum ReuseLambdaContextDecl_t { ReuseLambdaContextDecl };
----------------
Can you model this as an `ExpressionEvaluationContextRecord::ExpressionKind` instead of a new parameter?


================
Comment at: clang/lib/Sema/Sema.cpp:1026
 
+  HandleImmediateInvocations(ExprEvalContexts.back());
+
----------------
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.


================
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;
----------------
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.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:17062
     ExitDeclaratorContext(S);
+  isConstantEvaluatedOverride = false;
 }
----------------
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.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16107-16109
+  if (IsLambdaParamScope)
+    ExprEvalContexts.back().IIEndScopeAction =
+        Sema::ExpressionEvaluationContextRecord::IIESA_Propagate;
----------------
I'm a little uncomfortable about assuming that the innermost `ExpressionEvaluationContext` surrounding a lambda parameter is always a context for the lambda expression itself -- that's making a lot of assumptions about what contexts might be added in the future.

Instead of propagating this information from the lambda parameter context to the (presumed) lambda context when we pop an expression evaluation context for a lambda parameter, could we pull out the `LambdaScopeInfo` for the current lambda (`getCurLambda()`) and store the information there, to be either diagnosed or dropped when we reach `ActOnStartOfLambdaDefinition`?


================
Comment at: clang/lib/Sema/SemaLambda.cpp:895-896
+  if (ParamInfo.getDeclSpec().getConstexprSpecifier() == CSK_consteval)
+    ExprEvalContexts.back().IIEndScopeAction =
+        ExpressionEvaluationContextRecord::IIESA_Drop;
+
----------------
We've already parsed the default arguments at this point; it would seem simpler to directly clear out the immediate invocation state from the `ExpressionEvaluationContextRecord` rather than asking it to do that at the end of the scope. This would also remove the need for the `IIESA_Drop` mode entirely; if you also change the `IsLambdaParamScope` flag into an `ExpressionKind` on the context record, that would remove the need for the `IIEndScopeAction` field, given the information you need at the end of a scope is whether the `ExpressionKind` is that of a lambda parameter or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74130