[PATCH] D63960: [C++20] Add consteval-specific semantic for functions

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

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Some nits and very minor things, but I think this is generally good to go with those fixed. Sorry for the long review delay, and thanks for working on it!



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2394
+def err_invalid_consteval_call : Error<
+  "call to consteval function '%0' could not be evaluated">;
+def err_invalid_consteval_decl_kind : Error<
----------------
could not be evaluated -> is not a constant expression


================
Comment at: clang/include/clang/Sema/Sema.h:814
 
+  /// Whether the AST is currently being rebuild to correct immediate
+  /// invocations. Immediate invocation candidates and references to consteval
----------------
rebuild -> rebuilt


================
Comment at: clang/include/clang/Sema/Sema.h:1078
 
+    /// Set of candidate for starting an immediate invocation.
+    llvm::SmallVector<ImmediateInvocationCandidate, 4> ImmediateInvocationCandidates;
----------------
candidate -> candidates


================
Comment at: clang/include/clang/Sema/Sema.h:1081
+
+    /// Set of DeclRefExpr referencing a consteval function when used in a
+    /// context not already know to be immediately invoked.
----------------
DeclRefExpr -> DeclRefExprs


================
Comment at: clang/include/clang/Sema/Sema.h:1082
+    /// Set of DeclRefExpr referencing a consteval function when used in a
+    /// context not already know to be immediately invoked.
+    llvm::SmallPtrSet<DeclRefExpr *, 4> ReferenceToConsteval;
----------------
know -> known


================
Comment at: clang/lib/AST/ExprConstant.cpp:13618
+  if (InPlace) {
+    LValue LVal;
+    if (!::EvaluateInPlace(Result.Val, Info, LVal, this) ||
----------------
Tyker wrote:
> rsmith wrote:
> > This isn't sufficient: the evaluation process can refer back to the object under construction (eg, via `this`), and we need an lvalue that actually names the result in order for this to work.
> > 
> > I think you'll need to do something like creating a suitable object (perhaps a `LifetimeExtendedTemporaryDecl`) in the caller to represent the result of the immediate invocation, and passing that into here.
> i believe this solution should work. without LifetimeExtendedTemporaryDecl because reference/pointer on temporaries are not valid results of constant evaluation. so the AST should never store an APValue whose LValue is a ConstantExpr.
OK, I think that should be fine. Please update the comment above `class LValueExprEvaluator` to describe this additional case.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:8883
             << ConstexprKind;
+        NewFD->setConstexprKind(CSK_unspecified);
+      }
----------------
In C++20, we should probably reset to `constexpr` rather than unspecified. Before C++20, it's better for error recovery to leave the function as `constexpr`. (So unconditionally using `CSK_constexpr` here would be OK.)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15225
+    FD->printQualifiedName(OS, SemaRef.getASTContext().getPrintingPolicy());
+    SemaRef.Diag(CE->getBeginLoc(), diag::err_invalid_consteval_call) << Buffer;
+    for (auto &Note : Notes)
----------------
You can use `"%q0" to get a qualified name in a diagnostic; you don't need to turn it into a string yourself.


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

https://reviews.llvm.org/D63960