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

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

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Only minor comments remain (other than the `-Wc++17-compat` warning). In the interest of incremental progress, let's leave the `-Wc++17-compat` warning for a later patch; feel free to commit this after fixing up the other things.

(If you don't have commit access yet, now would be a good time to request it; please read http://llvm.org/docs/DeveloperPolicy.html for details.)

Thanks again!



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1598
+def err_consteval_non_function : Error<
+  "'virtual' can only appear on functions and constructors">;
 def err_virtual_non_function : Error<
----------------
Should this say `consteval` rather than `virtual`?

Actually... looks like this new diagnostic is not used, and could just be removed?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7860
+  "defaulted declaration of %sub{select_special_member_kind}0 "
+  "cannot be consteval because implicit definition is not be constexpr">;
 def warn_defaulted_method_deleted : Warning<
----------------
Remove stray "be" here.


================
Comment at: clang/lib/AST/ExprConstant.cpp:4561
   // Can we evaluate this function call?
-  if (Definition && Definition->isConstexpr() && Body)
+  if (Definition && Definition->isConstexpr() && !Definition->isInvalidDecl() &&
+      Body)
----------------
The `Definition && [...] Definition->isInvalidDecl()` case was handled a few lines above; this change appears to have no effect.


================
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);
----------------
Tyker wrote:
> rsmith wrote:
> > We should produce a `-Wc++17-compat` diagnostic similar to this for uses of `consteval`.
> a consteval keyword can only be lexed when we are in C++2a because it is a C++2a keyword so the warning would never fire.
The purpose of this warning is to warn about code parsed in C++2a mode that would not be valid in C++17. Try (eg) enabling `-Wc++98-compat` and building some C++11 code to see what's supposed to happen.


================
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);
 
----------------
Tyker wrote:
> rsmith wrote:
> > For `consteval` we should produce an "incompatible with C++ standards before C++2a" diagnostic, not an "incompatible with C++98" diagnostic.
> same as previous comment. the consteval keyword cannot be lexed unless we are in c++2a mode so the warning would never fire.
(See previous reply.)


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

https://reviews.llvm.org/D61790