[PATCH] D61790: [C++20] add Basic consteval specifier

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

rsmith added a comment.

Thank you!



================
Comment at: clang/include/clang/AST/DeclBase.h:1497
+
+    /// kind of Contexpr specifier as defined by ConstexprSpecKind.
+    uint64_t ConstexprKind : 2;
----------------
"kind" -> "Kind"
"Contexpr" -> "constexpr"


================
Comment at: clang/lib/AST/DeclPrinter.cpp:615
+      Out << "constexpr ";
+    if (D->isConsteval() && !D->isExplicitlyDefaulted())
+      Out << "consteval ";
----------------
The only way a defaulted function can be `consteval` is if `consteval` was literally written on the declaration, so I think we should print out the `consteval` keyword for all `consteval` functions regardless of whether they're defaulted.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2491
+  if (DS.hasConstexprSpecifier() && DSC != DeclSpecContext::DSC_condition) {
     Diag(DS.getConstexprSpecLoc(), diag::err_typename_invalid_constexpr);
     DS.ClearConstexprSpec();
----------------
Should this say which specifier was used? Or do we somehow reject eg. `sizeof(consteval int)` before we get here?


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1152-1154
     P.Diag(ConstexprLoc, !P.getLangOpts().CPlusPlus17
                              ? diag::ext_constexpr_on_lambda_cxx17
                              : diag::warn_cxx14_compat_constexpr_on_lambda);
----------------
We should produce a `-Wc++17-compat` diagnostic similar to this for uses of `consteval`.


================
Comment at: clang/lib/Sema/DeclSpec.cpp:1296-1297
       << (TypeSpecType == TST_char16 ? "char16_t" : "char32_t");
-  if (Constexpr_specified)
+  if (hasConstexprSpecifier())
     S.Diag(ConstexprLoc, diag::warn_cxx98_compat_constexpr);
 
----------------
For `consteval` we should produce an "incompatible with C++ standards before C++2a" diagnostic, not an "incompatible with C++98" diagnostic.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:4300
+    // the declaration of a function or function template
+    bool isConsteval = DS.getConstexprSpecifier() == CSK_consteval;
     if (Tag)
----------------
Please capitalize this variable name.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:6664
+        Diag(D.getDeclSpec().getConstexprSpecLoc(),
+             diag::err_constexpr_no_declarators)
+            << /*consteval*/ 1;
----------------
Please consider renaming this diagnostic; `err_constexpr_no_declarators` doesn't describe the problem here. Maybe `err_constexpr_wrong_decl_kind` or something?


================
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.
----------------
rsmith wrote:
> 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?
> 
I don't think the change here has really addressed the problem. We now say:

`error: defaulted definition of <thing> is not consteval`

in this case, which doesn't explain what's wrong, and appears to directly contradict what the user wrote. The problem is that the implicit definition would not be constexpr, so we should say that.


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

https://reviews.llvm.org/D61790