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

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

rsmith added a comment.

Very cool, this is an elegant approach.



================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:57
+def note_consteval_address_accessible : Note<
+  "%select{pointer|reference}0 on a consteval declaration "
+  "is not a constant expression">;
----------------
on -> to


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2366
+def err_invalid_consteval_decl_kind : Error<
+  "operator %select{new|delete|new[]|delete[]}0 cannot be declared consteval">;
 def err_invalid_constexpr : Error<
----------------
Please add 's around the function name. Instead of encoding the name as an integer, you can stream the declaration itself into the diagnostic and just use `"%0 cannot be declared consteval"` here.


================
Comment at: clang/include/clang/Sema/Sema.h:806-808
+  /// Should be set for most operation.
+  /// It is unset to disable tracking of immediate invocation candidates and
+  /// reference on consteval.
----------------
The comment should first describe what this field means, before saying how to use it (if necessary). So:

```
/// Whether immediate invocation candidates and references to consteval functions should be tracked.
```

or something like that.

That said, consider renaming this to `RebuildingImmediateInvocation` to avoid suggesting it's a general mechanism to turn off immediate invocation tracking.


================
Comment at: clang/include/clang/Sema/Sema.h:1072
+    llvm::SmallVector<llvm::PointerIntPair<ConstantExpr *, 1>, 8>
+        ImmediateInvocationsCandidates;
+
----------------
`ImmediateInvocationsCandidates` -> `ImmediateInvocationCandidates`


================
Comment at: clang/include/clang/Sema/Sema.h:1076
+    /// context not already know to be immediately invoked.
+    llvm::SmallPtrSet<DeclRefExpr *, 8> ReferenceOnConsteval;
+
----------------
`ReferenceOnConsteval` -> `ReferencesToConsteval` or similar.


================
Comment at: clang/lib/AST/ExprConstant.cpp:1932
+      if (FD->isConsteval()) {
+        Info.FFDiag(Loc, diag::note_consteval_address_accessible)
+            << !Type->isAnyPointerType();
----------------
It would be useful for the diagnostic to say what consteval function we have a pointer to, and maybe include a note pointing at its declaration.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11188-11189
 
+  ConstexprSpecKind ConstexprKind = DetermineSpecialMemberConstexprKind(
+      Constexpr, ClassDecl->hasDefaultedConstevalDefaultCtor());
+
----------------
I don't think this is necessary: implicit special members are never `consteval`. (We use `DeclareImplicitDefaultConstructor` when there is no user-declared default constructor, in which case there can't possibly be a defaulted consteval default constructor.) I don't think you need any of the `DetermineSpecialMemberConstexprKind` machinery, nor the tracking of `DefaultedSpecialMemberIsConsteval` in `CXXRecordDecl`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15032
+    return E;
+  if (auto *Call = dyn_cast<CallExpr>(E.get()))
+    if (auto *DeclRef =
----------------
rsmith wrote:
> Please add a comment to this to point out that it's just an optimization, for example:
> 
> "Opportunistically remove the callee from ReferencesToConsteval if we can. It's OK if this fails; we'll also remove this in HandleImmediateInvocations, but catching it here allows us to avoid walking the AST looking for it in simple cases."
Consider using `E.get()->IgnoreImplicit()` here; we want to step over (at least) any `CXXBindTemporaryExpr`s inserted by `MaybeBindToTemporary`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15032-15035
+  if (auto *Call = dyn_cast<CallExpr>(E.get()))
+    if (auto *DeclRef =
+            dyn_cast<DeclRefExpr>(Call->getCallee()->IgnoreImplicit()))
+      ExprEvalContexts.back().ReferenceOnConsteval.erase(DeclRef);
----------------
Please add a comment to this to point out that it's just an optimization, for example:

"Opportunistically remove the callee from ReferencesToConsteval if we can. It's OK if this fails; we'll also remove this in HandleImmediateInvocations, but catching it here allows us to avoid walking the AST looking for it in simple cases."


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15036-15039
+  ConstantExpr *Res = ConstantExpr::Create(
+      getASTContext(), E.get(),
+      ConstantExpr::getStorageKind(E.get()->getType().getTypePtr(),
+                                   getASTContext()));
----------------
We will need an `ExprWithCleanups` wrapping `E.get()` if there might be any temporaries within it.

We don't actually know whether there definitely are any such temporaries, but we can conservatively assume we might need an `ExprWithCleanups` if `Cleanup.exprNeedsCleanups()` returns `true`.

You can test this with something like:

```
struct A { int *p = new int(42); consteval int get() { return *p; } constexpr ~A() { delete p; } };
int k = A().get();
```

... which should be valid, but will be rejected (or might assert) if we don't destroy the `A()` temporary as part of evaluating the `consteval` call to `get`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15071
+                    llvm::PointerIntPair<ConstantExpr *, 1> RHS) {
+      return LHS.getPointer()->getBeginLoc() < RHS.getPointer()->getBeginLoc();
+    }
----------------
`<` on `SourceLocation` doesn't really mean anything. It's not correct to use it here. Also, using source locations will not give you outer expressions before inner ones. Consider:

```
enum E {};
consteval E f();
consteval int operator+(E, int);
void g() {
  f() + 1;
}
```

 Here, the begin location of both the call to `f()` and the call to `operator+` are in the same place: at the `f` token.

I don't think you need this sort at all: instead, you can process immediate invocations in `ImmediateInvocationCandidates` in reverse order: subexpressions must have been built before their enclosing expressions, so walking in reverse order will guarantee that you process an outer candidate before any inner ones.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15102
+                ExprCompType{});
+            if (It != IISet.end()) {
+              It->setInt(1); // Mark as deleted
----------------
We should look for an exact match here, not merely for something nearby. (Maybe: form a pointer set of immediate invocation pointer expressions, and remove things from the set when you encounter them in this walk; then when iterating over the immediate invocations, skip ones that have already been erased from the set.)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15118-15119
+          }
+          /// FIXME: Add an option to tree transfrom that prevents rebuilding
+          /// nodes that only need rewiring and use this option here.
+          bool AlwaysRebuild() { return false; }
----------------
I don't imagine this will ever actually happen, because immutability of the AST is so important to other parts of Clang. This should be a rare case, so it likely doesn't matter; I'd just remove this FIXME.


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

https://reviews.llvm.org/D63960