[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9410-9421
+def ext_incorrect_defaulted_comparison_constexpr : Extension<
   "defaulted definition of %select{%sub{select_defaulted_comparison_kind}1|"
   "three-way comparison operator}0 "
-  "cannot be declared %select{constexpr|consteval}2 because "
+  "declared %select{constexpr|consteval}2 but "
   "%select{it|the corresponding implicit 'operator=='}0 "
-  "invokes a non-constexpr comparison function">;
+  "invokes a non-constexpr comparison function are a C++2b extension">, InGroup<CXX2bDefCompCallsNonConstexpr>;
+def warn_cxx2b_incorrect_defaulted_comparison_constexpr : Warning<
----------------
rsmith wrote:
> The grammar of these diagnostics looks a bit peculiar. Suggested an alternative, but it's still a bit awkward ("...but for which..."), maybe you can find something better.
@aaron.ballman any suggestions for less awkward wording here? I don't think what there is here now is too bad but asking for a another opinion.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8808-8809
   if (FD->isConstexpr()) {
     if (CheckConstexprReturnType(*this, FD, CheckConstexprKind::Diagnose) &&
         CheckConstexprParameterTypes(*this, FD, CheckConstexprKind::Diagnose) &&
         !Info.Constexpr) {
----------------
rsmith wrote:
> We'll also be getting diagnostics here if the parameter and return types are not literal types. You can pass `...::CheckValid` instead of `...::Diagnose` to find out if there would be an error, but we should probably instead make the `CheckConstexpr...` functions produce warnings (`ExtWarn` prior to C++23) rather than errors in general. That's a much larger change, though -- there's probably half a dozen diagnostics in the `CheckConstexpr*` functions that will need to be updated to properly support P2448R2. If you just want to implement this one part of P2448 for now and leave the rest for a later change, that seems fine to me.
I don't think I have the bandwidth for a larger change but I can file a bug and assign myself to it and I will put in my work queue.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8813
+	  (getLangOpts().CPlusPlus2b ? diag::warn_cxx2b_incorrect_defaulted_comparison_constexpr :
+                                        diag::ext_incorrect_defaulted_comparison_constexpr ))
           << FD->isImplicit() << (int)DCK << FD->isConsteval();
----------------
rsmith wrote:
> Don't need the parens around this. This layout is a little unusual, what does clang-format do here?
My bad, I forgot to apply clang-format.


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

https://reviews.llvm.org/D146090