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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:91-92
   expressions and how their types are deduced therein, in all C++ language versions.
+- Implemented partial support for `P2448R2: Relaxing some constexpr restrictions <https://wg21.link/p2448r2>`_
+  Explicitly defaulted functions no longer have to be constexpr-compatible but merely constexpr suitable.
 
----------------
Should we also say what's still missing that causes this to be partial?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9415-9421
+  "invokes a non-constexpr comparison function is a C++2b extension">, InGroup<CXX2bDefCompCallsNonConstexpr>;
+def warn_cxx2b_compat_incorrect_defaulted_comparison_constexpr : Warning<
+  "defaulted definition of %select{%sub{select_defaulted_comparison_kind}1|"
+  "three-way comparison operator}0 that is "
+  "declared %select{constexpr|consteval}2 but"
+  "%select{|for which the corresponding implicit 'operator==' }0 "
+  "invokes a non-constexpr comparison function is a C++2b extension">, InGroup<CXXPre2bCompat>, DefaultIgnore;
----------------
Formatting fix-up and a wording correction.


================
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<
----------------
shafik wrote:
> 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.
Yeah, that wording is a bit... wordy, but I'm struggling to think of something more approachable that's still accurate.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8811
+  // The concept of constexpr-compatible was removed.
+  // C++23 [dcl.fct.def.default]/p3 [P2448R2]
+  //  A function explicitly defaulted on its first declaration is implicitly
----------------



================
Comment at: clang/test/CXX/class/class.compare/class.compare.default/p4.cpp:153
+
+    constexpr friend bool operator==(const my_struct &, const my_struct &) noexcept = default; // no diagnostic extension-warning {{declared constexpr but invokes a non-constexpr comparison function is a C++2b extension}}
+};
----------------
`no diagnostic extension-warning`?


================
Comment at: clang/test/CXX/class/class.compare/class.compare.default/p4.cpp:162
+
+my_struct<non_constexpr_type> obj; // extension-note {{in instantiation of template class 'GH61238::my_struct<GH61238::non_constexpr_type>' requested here}}
+}
----------------
Can you add an instantiation that does use a constexpr suitable type to show that we don't issue the diagnostic on that instantiation, or do you think that is sufficiently covered by other test coverage?


================
Comment at: clang/www/cxx_status.html:1485
       <td><a href="https://wg21.link/P2448R2">P2448R2</a></td>
-      <td class="none" align="center">No</td>
+      <td class="none" align="center">Partial</td>
     </tr>
----------------
Please add  <details> markup to explain why the support is partial, and it's probably not a bad idea to say `Clang 17 (Partial)` so users know when the partial support started.


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

https://reviews.llvm.org/D146090