[libcxx-commits] [PATCH] D100283: [libcxx] removes `weak_equality` and `strong_equality` from <compare>

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

I downloaded the raw diffs for D74186 <https://reviews.llvm.org/D74186> and D100283 <https://reviews.llvm.org/D100283>, applied them in separate git branches, and then did `git diff barry chris` to find the places they differed.
The diff is here, if you want to check my work and see if I missed anything: https://pastebin.com/XcAS19dU



================
Comment at: libcxx/include/compare:25-26
   // named comparison functions
-  constexpr bool is_eq  (weak_equality cmp) noexcept    { return cmp == 0; }
-  constexpr bool is_neq (weak_equality cmp) noexcept    { return cmp != 0; }
   constexpr bool is_lt  (partial_ordering cmp) noexcept { return cmp < 0; }
----------------
Instead of deleting these lines, change `weak_equality` to `partial_ordering`.
I think this was probably a cut-and-paste error in the original? Anyway, `is_eq(partial_ordering)` still exists and should be synopsized here.


================
Comment at: libcxx/include/compare:423
 
-  // conversions
-  _LIBCPP_INLINE_VISIBILITY
----------------
Please leave this comment line here.


================
Comment at: libcxx/include/compare:516-517
   _None,
   _WeakEq,
   _StrongEq,
   _PartialOrd,
----------------
Remove `_WeakEq` and `_StrongEq`.


================
Comment at: libcxx/include/compare:544-549
   if (__seen[_WeakEq])
     return _WeakEq;
   if (__seen[_StrongEq] && (__seen[_PartialOrd] || __seen[_WeakOrd]))
     return _WeakEq;
   if (__seen[_StrongEq])
     return _StrongEq;
----------------
Remove `_WeakEq` and `_StrongEq`.


================
Comment at: libcxx/include/compare:562
   constexpr _CCC _Cat = __compute_comp_type(__type_kinds);
-  if constexpr (_Cat == _None)
-    return void();
-  else if constexpr (_Cat == _WeakEq)
-    return weak_equality::equivalent;
-  else if constexpr (_Cat == _StrongEq)
-    return strong_equality::equivalent;
-  else if constexpr (_Cat == _PartialOrd)
+  if constexpr (_Cat == _PartialOrd)
     return partial_ordering::equivalent;
----------------
You still need the `(_Cat == _None)` case here: if any of the elements are noncomparable then the result of this function must be `void()`, not `strong_ordering::equivalent`.
(At first I was worried that we had no tests for this; but I see below we do have tests. Don't remove those tests.)
https://eel.is/c++draft/cmp.common#2


================
Comment at: libcxx/test/std/language.support/cmp/cmp.common/common_comparison_category.pass.cpp:43-51
-  // [class.spaceship]p4.1: If any Ti is not a comparison category tpe, U is void.
-  {
-    test_cat<void, void>();
-    test_cat<void, int*>();
-    test_cat<void, SO&>();
-    test_cat<void, SO const>();
-    test_cat<void, SO*>();
----------------
You cannot remove these tests. They need to keep passing in C++20.
https://eel.is/c++draft/cmp.common#2



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100283