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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
