[PATCH] D61790: [C++20] add Basic consteval specifier

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10471
 
+  bool IsConstantContext = S.ExprEvalContexts.back().isConstantEvaluated();
+
----------------
rsmith wrote:
> Are the changes to this file really specific to `consteval` evaluation? They look more general than that; I think this would fix the evaluation of `std::is_constant_evaluated()` in any constant-evaluated context that we analyzed. Can this be split out of this patch into a separate change?
this can be splited in and other patch. https://reviews.llvm.org/D62009.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5720-5769
+bool Sema::CheckInvalidConstevalCall(FunctionDecl *FDecl, SourceLocation Loc,
+                                     ArrayRef<Expr *> Args, Expr *This) {
+  bool Failed = false;
+  if (FDecl && FDecl->isConsteval() &&
+      !ExprEvalContexts.back().isConstantEvaluated()) {
+
+    SmallVector<PartialDiagnosticAt, 8> Diags;
----------------
rsmith wrote:
> This checking doesn't make sense: we should be checking that the invocation as a whole is a constant expression, not whether the arguments (evaluated in isolation) are constant expressions. This check should be applied to the `Expr` representing the call, and should wrap it in a `ConstantExpr` holding the evaluated value if evaluation succeeds.
the idea behind this check was "if the function satisfies the rules of constexpr and all it arguments can be constant evaluated. this function call can be constant evaluated". this was a easy way of checking that the call could be evaluated without evaluating it.

this will be added in later patch after https://reviews.llvm.org/D62399.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:13417-13435
+bool Sema::CheckInvalidConstevalTakeAddress(Expr *Input) {
+  if (Input) {
+    if (auto *DeclRef = dyn_cast<DeclRefExpr>(Input->IgnoreParens())) {
+      if (auto *Function = dyn_cast<FunctionDecl>(DeclRef->getDecl())) {
+        if (Function->isConsteval()) {
+          const char *DeclName = Function->isOverloadedOperator()
+                                     ? "consteval operator"
----------------
rsmith wrote:
> This isn't correct: you can take the address of a `consteval` function within an immediate invocation. In general, we need to delay this checking until we've finished processing any potentially-enclosing immediate invocations. Trying to capture all of the places where we might form a reference to a `consteval` function name without forming an immediate invocation is also fragile: you'll miss some, and as Clang is extended, there'll be more cases introduced that you miss.
> 
> Here's how I was intending to handle this:
> 
>  * Change `Sema::MarkFunctionReferenced` to take an `Expr*` instead of a location, and update all of its callers. (We'll need a separate function for marking destructors referenced, because destructor calls typically don't have expressions, but destructors can't be `consteval` so that's OK.)
>  * In `Sema::MarkFunctionReferenced`, if `Func` is a `consteval` function and our current context is not a `consteval` function, add it to a list on the `ExpressionEvaluationContext`.
>  * When forming an immediate invocation, walk all of its subexpressions and remove them all from the list on the `ExpressionEvaluationContext`
>  * When popping the `ExpressionEvaluationContext`, diagnose any remaining expressions in its list.
> 
> (The first step should be done in a separate patch to keep the diffs smaller and easier to review.)
delayed to later patch.


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

https://reviews.llvm.org/D61790