[libc++][spaceship] Fixed `__debug_three_way_comp`'s `operator()` for `vector<bool>'s `operator<=>`

[H-G-Hristov]

Summary

An issue with operator() was found during the implementation of https://reviews.llvm.org/D132268.

This patch aims to resolve the issues by updating the operator to use perfect forwarding.

The original motivation for three_way_comp_ref_type is given in: https://reviews.llvm.org/D131395

three_way_comp_ref_type's implementation is inspired by comp_ref_type, which has two overloads:

template <class _Tp, class _Up>
bool operator()(const _Tp& __x,  const _Up& __y);

template <class _Tp, class _Up>
bool operator()(_Tp& __x,  _Up& __y);

__debug_three_way_comp is missing the first overload and also declares the typealias`_three_way_comp_ref_type ` incorrectly.

[H-G-Hristov]

Try to fix CI

[H-G-Hristov]

Try to fix CI

[H-G-Hristov]

Try to fix CI

[H-G-Hristov]

Try to fix CI

[H-G-Hristov]

Try to fix CI

[H-G-Hristov]

Try to fix CI

[H-G-Hristov]

• Use "perfect forwarding"

[H-G-Hristov]

@philnik Could you please have a look at this patch?

[Mordante]

libcxx/include/__algorithm/three_way_comp_ref_type.h

39

"three_way_comp_ref_type's implementation is inspired by comp_ref_type, which has two overloads:"

I'm a bit confused. Here you don't use two overloads. Can you explain the difference between the message and the implementation?

[H-G-Hristov]

libcxx/include/__algorithm/three_way_comp_ref_type.h

39

I replaced the two overloads in comp_ref_type with the perfect forwarding version as philnik recommended, which should has the same effect, right? Maybe I need to change the message?

[H-G-Hristov]

libcxx/include/__algorithm/three_way_comp_ref_type.h

39

Or would the implementation with the two overloads as comp_ref_type more appropriate? I am not sure entirely.

[H-G-Hristov]

@philnik @Mordante ping

[H-G-Hristov]

This comment was removed by H-G-Hristov.

[H-G-Hristov]

Rebased

[H-G-Hristov]

ping

[philnik]

LGTM % nit.

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp

158–164

It's at most that number of comparisons, so we shouldn't check for equality anyways.

[H-G-Hristov]

• Addressed comments

[H-G-Hristov]

@philnik Thank you for the review!

libcxx/include/__algorithm/three_way_comp_ref_type.h

 #if _LIBCPP_STD_VER >= 20
 
 template <class _Comp>
 struct __debug_three_way_comp {
   _Comp& __comp_;
   _LIBCPP_HIDE_FROM_ABI constexpr __debug_three_way_comp(_Comp& __c) : __comp_(__c) {}
 
   template <class _Tp, class _Up>
+  _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(const _Tp& __x, const _Up& __y) {
+    auto __r = __comp_(__x, __y);
+    __do_compare_assert(0, __y, __x, __r);
+    return __r;
+  }
+
+  template <class _Tp, class _Up>
   _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp& __x, _Up& __y) {
     auto __r = __comp_(__x, __y);
     __do_compare_assert(0, __y, __x, __r);
     return __r;
   }
 
   template <class _LHS, class _RHS, class _Order>
   _LIBCPP_HIDE_FROM_ABI constexpr inline void __do_compare_assert(int, _LHS& __l, _RHS& __r, _Order __o)
     requires __comparison_category<decltype(std::declval<_Comp&>()(std::declval<_LHS&>(), std::declval<_RHS&>()))>
   {
struct __debug_three_way_comp {
   }
 
   template <class _LHS, class _RHS, class _Order>
   _LIBCPP_HIDE_FROM_ABI constexpr inline void __do_compare_assert(long, _LHS&, _RHS&, _Order) {}
 };
 
 // Pass the comparator by lvalue reference. Or in debug mode, using a
 // debugging wrapper that stores a reference.
-#  ifndef _LIBCPP_ENABLE_DEBUG_MODE
+#  ifdef _LIBCPP_ENABLE_DEBUG_MODE
 template <class _Comp>
 using __three_way_comp_ref_type = __debug_three_way_comp<_Comp>;
 #  else
 template <class _Comp>
 using __three_way_comp_ref_type = _Comp&;
 #  endif
 
 #endif // _LIBCPP_STD_VER >= 20
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ALGORITHM_THREE_WAY_COMP_REF_TYPE_H

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp

constexpr void test_comparator_invocation_count() {
   // If one of the ranges is empty, the comparator must not be called at all
   compare_invocation_count = 0;
   test_lexicographical_compare<const int*, const int*>(
       std::array{0, 1, 2, 3}, std::array<int, 0>{}, compare_last_digit_counting, std::strong_ordering::greater);
   assert(compare_invocation_count == 0);
   // The comparator is invoked only `min(left.size(), right.size())` times
   test_lexicographical_compare<const int*, const int*>(
       std::array{0, 1, 2}, std::array{0, 1, 2, 3}, compare_last_digit_counting, std::strong_ordering::less);
-  assert(compare_invocation_count == 3);
+  // In debug mode the comparator will be called twice by the debugging wrapper `__debug_three_way_comp`
+  int expected_compare_invocation_count = 3;
+#ifdef _LIBCPP_ENABLE_DEBUG_MODE
+  expected_compare_invocation_count *= 2;
+#endif
+  assert(compare_invocation_count == expected_compare_invocation_count);
 }
 
 constexpr bool test() {
   test_iterator_types();
   test_comparison_categories();
   test_comparator_invocation_count();
 
   return true;
 }
 
 int main(int, char**) {
   test();
   static_assert(test());
 
   return 0;
 }