rsmith added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2317
+def err_invalid_consteval_take_address : Error<
+ "cannot take address of consteval function %0 in non-constexpr context">;
+def err_consteval_address_accessible : Error<
----------------
Let's use the proper terminology here so people can search for it: "outside of
an immediate invocation"
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2318-2320
+def err_consteval_address_accessible : Error<
+ "%select{return value|constructed object}0 contain pointer on consteval
declaration;"
+ " this would make it accessible at runtime">;
----------------
This diagnostic should be moved to DiagnosticASTKinds (and produced by
CheckConstantExpression; see my other comment on this). Please rephrase this to
follow the same pattern as `note_constexpr_non_global`.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2324
+def err_invalid_consteval_decl_kind : Error<
+ "operator %select{new|delete|new[]|delete[]}0 cannot be consteval
specified">;
def err_invalid_constexpr : Error<
----------------
"consteval specified" -> "declared consteval"
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3771-3772
"candidate function made ineligible by enable_if">;
+def note_addrof_ovl_consteval : Note<
+ "candidate function made ineligible by consteval specifier">;
def note_ovl_candidate_deduced_mismatch : Note<
----------------
`consteval` should not affect overload resolution. I don't think this
diagnostic makes sense. (We should first pick the function and *then* check
whether it's usable.)
================
Comment at: clang/include/clang/Sema/Sema.h:2152
+ ExprResult ActOnConstevalCall(FunctionDecl *FD, Expr *Call);
+
----------------
This should be called `CheckImmediateInvocation` or similar. `ActOn*` functions
should only be called by the parser in response to source constructs, not
internally by `Sema`.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:2232
!Expr::isPotentialConstantExpr(Dcl, Diags)) {
- SemaRef.Diag(Dcl->getLocation(),
- diag::ext_constexpr_function_never_constant_expr)
- << isa<CXXConstructorDecl>(Dcl);
+ SemaRef.Diag(Dcl->getLocation(),
diag::ext_constexpr_function_never_constant_expr)
+ << isa<CXXConstructorDecl>(Dcl) << Dcl->isConsteval();
----------------
Please keep lines to 80 columns or fewer.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:5599-5604
+ExprResult Sema::ActOnConstevalCall(FunctionDecl *FD, Expr *Call) {
+ Expr::EvalResult Result;
+ llvm::SmallVector<PartialDiagnosticAt, 8> Diags;
+ Result.Diag = &Diags;
+ Call->EvaluateAsConstantExpr(Result, Expr::EvaluateForCodeGen, Context);
+ if (!Result.Diag->empty()) {
----------------
I don't think this approach can work as-is. You need to know the complete
bounds of the immediate invocation before you can check it. For example:
```
enum E {};
consteval int operator+(int (*f)(), E) { return f(); }
consteval int fn() { return 42; }
consteval auto get() -> int(*)() { return fn; }
int k = get() + E();
```
This is (or at least should be) valid code: the immediate invocation here is
the entire `get() + E()` expression, and evaluating that produces `42`. But
this approach will reject the code, because `get()` in isolation is a constant
expression. (The current wording suggests that this code is invalid, but I'm
trying to get that fixed; I'm confident that was not the intent.)
I think we need a different approach. Perhaps:
* when we see a candidate immediate invocation, immediately create a
`ConstantExpr` wrapping it, but don't evaluate it yet
* add that `ConstantExpr` to a list on the expression evaluation context record
* if there already were any expressions listed there, check to see if any of
them is a subexpression of the current expression, and if so, perform a
`TreeTransform` to remove the `ConstantExpr` wrappers from those subexpressions
and remove them from the list on the evaluation context record
* when we pop an expression evaluation context record, *then* evaluate all
remaining `ConstantExpr`s for immediate invocations and check they're valid
(Or some other approach that handles this wrinkle; the above is just a
suggestion.)
================
Comment at: clang/lib/Sema/SemaExpr.cpp:5612-5616
+ if (CheckPointerOnConsteval(Result.Val)) {
+ Diag(Call->getBeginLoc(), diag::err_consteval_address_accessible)
+ << isa<CXXConstructorDecl>(FD);
+ return ExprError();
+ }
----------------
This should be checked when determining whether the core constant expression is
a constant expression, which happens in `CheckConstantExpression` in
`ExprConstant.cpp`, not here.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:5620
+
ExprResult Sema::ActOnCallExpr(Scope *Scope, Expr *Fn, SourceLocation
LParenLoc,
MultiExprArg ArgExprs, SourceLocation RParenLoc,
----------------
This is the wrong place to call `ActOnConstevalCall` from: that needs to happen
when we actually build the resolved call expression (which isn't always done by
this function).
================
Comment at: clang/lib/Sema/SemaExpr.cpp:5641-5642
+ if (CallExpr *CExpr = dyn_cast<CallExpr>(Call.get()))
+ if (auto *FD = dyn_cast_or_null<FunctionDecl>(CExpr->getCalleeDecl()))
+ if (FD && FD->isConsteval() && !isConstantEvaluated())
+ Call = ActOnConstevalCall(FD, CExpr);
----------------
`!isConstantEvaluated()` should be checked in the `CXXConstructExpr` case too;
consider moving that check into `ActOnConstevalCall`.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:5761-5762
// in ArgExprs.
- if ((FDecl =
- rewriteBuiltinFunctionDecl(this, Context, FDecl, ArgExprs))) {
+ if ((FDecl = rewriteBuiltinFunctionDecl(this, Context, FDecl, ArgExprs,
+ /*Diagnose*/ false))) {
NDecl = FDecl;
----------------
What's the purpose of this change?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:9596-9607
+ if (!S.isConstantEvaluated() && FD->isConsteval()) {
+ if (Complain) {
+ if (InOverloadResolution)
+ S.Diag(FD->getBeginLoc(), diag::note_addrof_ovl_consteval);
+ else {
+ S.Diag(Loc, diag::err_invalid_consteval_take_address) << FD;
+ S.Diag(FD->getBeginLoc(), diag::note_declared_at);
----------------
This isn't the right way to deal with this, in part because we don't know
whether we're in an immediate invocation or not at this point. Instead, we
should track that we saw a use of a `consteval` function from
`Sema::DiagnoseUseOfDecl`, and issue a diagnostic if that use is not within an
immediate invocation / within a consteval function.
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
