rsmith added a comment.
Thanks for working on this!
Comments on the general approach:
- We should only evaluate each immediate invocation once (this will become
essential once we start supporting reflection -- and particularly operations
that mutate the AST -- inside `consteval` functions).
- Each time an immediate invocation is formed, you should create a
`ConstantExpr` AST node wrapping that call to reflect that it is a constant.
- You should change `ConstantExpr` to store an `APValue` representing the
evaluated value of the expression.
Most of the above should be done as a separate preparatory change; we should be
able to remove a lot of the existing ad-hoc caching of evaluated values (eg,
for enumerators and the initializers of variables) at the same time.
Please also split this into smaller pieces. If you split off a patch to just
add the keyword, parsing, and AST representation for the `consteval` specifier
(but treat it identically to `constexpr` for constant evaluation purposes),
that'd be a good first patch for this feature.
================
Comment at: clang/include/clang/AST/Decl.h:2102
/// Whether this is a (C++11) constexpr function or constexpr constructor.
bool isConstexpr() const { return FunctionDeclBits.IsConstexpr; }
void setConstexpr(bool IC) { FunctionDeclBits.IsConstexpr = IC; }
----------------
Please rename this to `isConstexprSpecified()` (compare this to how we model
`inline`, which has similar behavior: it can be explicit, or can be implied by
other properties of the function).
================
Comment at: clang/include/clang/AST/Decl.h:2109
+
+ bool isConstexprOrConsteval() const { return isConstexpr() || isConsteval();
}
+
----------------
Both the `constexpr` and `consteval` specifiers make a function a "constexpr
function", so I think this should be called simply `isConstexpr`; callers that
want to distinguish `constexpr` from `consteval` can use `isConsteval()` (or we
can add a `getConstexprKind()` or similar function).
================
Comment at: clang/include/clang/AST/DeclCXX.h:2115
+ QualType T, TypeSourceInfo *TInfo, StorageClass SC,
+ bool isInline, bool isConstexpr, bool isConsteval,
+ SourceLocation EndLocation)
----------------
Instead of the three bool flags in a row here (which will make call sites hard
to read), consider using an enum, perhaps:
```
enum class ConstexprSpecifierKind { None, Constexpr, Consteval };
```
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2297-2300
+def err_consteval_cannot_be_constant_eval : Error<
+ "call to %0 cannot be constant evaluated">;
+def note_argument_n_cannot_be_constant_eval : Note<
+ "argument %0 cannot be constant evaluated">;
----------------
It would be useful in these diagnostics to explain why the call is required to
be constant evaluated; please also use the standard terminology "constant
expression" here. (Eg, "call to consteval function %0 is not a constant
expression")
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2308-2314
def err_constexpr_tag : Error<
"%select{class|struct|interface|union|enum}0 cannot be marked constexpr">;
-def err_constexpr_dtor : Error<"destructor cannot be marked constexpr">;
+def err_constexpr_dtor : Error<"destructor cannot be marked %0">;
+def err_invalid_consteval_decl_kind : Error<
+ "operator %select{new|delete|new[]|delete[]}0 cannot be marked consteval">;
+def err_take_adress_of_consteval_decl : Error<
+ "taking address of a %0">;
----------------
In these diagnostics (including the existing one for tags), we should say
"declared constexpr" not "marked constexpr".
================
Comment at: clang/include/clang/Basic/TokenKinds.def:390-391
+// C++ consteval proposal
+CXX2A_KEYWORD(consteval , 0)
+
----------------
This and `char8_t` are both adopted C++2a features now (they'e not just
proposals any more); please change the comment to just "C++2a keywords".
================
Comment at: clang/include/clang/Sema/DeclSpec.h:404
SourceLocation FS_forceinlineLoc;
- SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc;
+ SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc, ConstevalLoc;
SourceLocation TQ_pipeLoc;
----------------
I think it would be preferable to track only one location here (for the
`constexpr` / `consteval` specifier) and store a constexpr specifier kind
instead of `Constexpr_specified`, like we do for the other kinds of specifier
for which we allow only one of a set of keywords.
================
Comment at: clang/include/clang/Sema/Sema.h:985-986
+ /// cases in a switch statement).
+ /// - "immediate function context" in C++2a terms, a call to a function
+ /// marked as consteval
ConstantEvaluated,
----------------
An "immediate function context" is also "potentially evaluated"; I think what
we want to say here is something like "The current context is "potentially
evaluated", but will only ever be constant evaluated; runtime code will never
be generated for it. (Eg, the case values of a switch, or the body of a
consteval function.)"
================
Comment at: clang/include/clang/Sema/Sema.h:4052-4053
+ /// CheckInvalidConstevalCall - detect and diagnose invalid calls to
+ /// consteval function.
+ /// return true if the call is invalid.
----------------
Please don't use the "FunctionName - " style in new code.
================
Comment at: clang/include/clang/Sema/Sema.h:4054
+ /// consteval function.
+ /// return true if the call is invalid.
+ bool CheckInvalidConstevalCall(FunctionDecl *FDecl, SourceLocation Loc,
----------------
return -> \return
================
Comment at: clang/include/clang/Sema/Sema.h:4055-4056
+ /// return true if the call is invalid.
+ bool CheckInvalidConstevalCall(FunctionDecl *FDecl, SourceLocation Loc,
+ ArrayRef<Expr *> Args, Expr *This = nullptr);
+
----------------
Drop the "Invalid" from this function name; it doesn't add anything. But I
think this is the wrong interface anyway -- I'll get to that later.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4474-4475
const LValue *ResultSlot) {
+ EvalInfo::EnterConstantContextRAII ContextRAII(
+ Info, Callee->isConsteval() || Info.InConstantContext);
ArgVector ArgValues(Args.size());
----------------
We should not need to do this; instead, when the evaluator hits a
`ConstantExpr` node, it should use the already-evaluated value of that node.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2480-2487
+ Diag(DS.getInlineSpecLoc(), diag::err_typename_invalid_specifier)
+ << "function";
if (DS.isVirtualSpecified())
- Diag(DS.getVirtualSpecLoc(), diag::err_typename_invalid_functionspec);
+ Diag(DS.getVirtualSpecLoc(), diag::err_typename_invalid_specifier)
+ << "function";
if (DS.hasExplicitSpecifier())
+ Diag(DS.getExplicitSpecLoc(), diag::err_typename_invalid_specifier)
----------------
Do not pass random English words into diagnostics; this makes translation
impossible. Use a `%select` or two different diagnostics instead. (Passing in
C++ syntax such as `constexpr` or `consteval` is OK, since they don't need to
be translated.)
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1073-1075
+ case tok::identifier:
+ if (P.getCurToken().getIdentifierInfo()->getName() != "consteval")
+ return;
----------------
It doesn't seem worthwhile to me to support `consteval` in earlier language
modes only for lambdas; if we really want to allow `consteval` as an extension
in earlier language modes, the better approach would be to add a `__consteval`
keyword. But regardless of whether we do that, please remove this.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1303-1305
+ case tok::kw_consteval:
+ TokKind = 4;
+ break;
----------------
Please follow the formatting of the adjacent code.
================
Comment at: clang/lib/Sema/DeclSpec.cpp:1326-1332
+ // C++2a [dcl.spec]p2: The constexpr and consteval decl-specifiers shall not
+ // both appear in a decl-specifier-seq.
+ if (isConstevalSpecified() && isConstexprSpecified()) {
+ S.Diag(ConstexprLoc, diag::err_incompatible_constexpr_consteval)
+ << FixItHint::CreateRemoval(ConstexprLoc);
+ Constexpr_specified = false;
+ }
----------------
This should be handled by `SetConstevalSpec` instead; please look for calls to
`BadSpecifier` to see how we handle this for other kinds of specifier.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:10471
+ bool IsConstantContext = S.ExprEvalContexts.back().isConstantEvaluated();
+
----------------
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?
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:640-648
if (New->isConstexpr() != Old->isConstexpr()) {
Diag(New->getLocation(), diag::err_constexpr_redecl_mismatch)
- << New << New->isConstexpr();
+ << New << New->isConstexpr() << "constexpr";
+ Diag(Old->getLocation(), diag::note_previous_declaration);
+ Invalid = true;
+ } else if (New->isConsteval() != Old->isConsteval()) {
+ Diag(New->getLocation(), diag::err_constexpr_redecl_mismatch)
----------------
This will produce diagnostics such as "non-constexpr declaration follows
constexpr declaration" for a `consteval` declaration following a `constexpr`
declaration, which seems misleading since a `consteval` declaration does
declare a `constexpr` function. I think the order of these two checks should be
reversed, so that that case will instead be diagnosed as "consteval declaration
follows non-consteval declaration".
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6655
MD->getTemplatedKind() == FunctionDecl::TK_NonTemplate) {
Diag(MD->getBeginLoc(), diag::err_incorrect_defaulted_constexpr) << CSM;
// FIXME: Explain why the special member can't be constexpr.
----------------
The diagnostic text here is:
```
error: defaulted definition of <thing> is not constexpr
```
which might be confusing if the user wrote `consteval thing = default;`. Maybe
change the diagnostic to:
"defaulted definition of <thing> cannot be declared
%select{constexpr|consteval}1 because its implicit definition would not be
constexpr"
or something like that?
================
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;
----------------
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.
================
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"
----------------
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.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61790/new/
https://reviews.llvm.org/D61790
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
