[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

32

"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

32

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

32

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–166

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___ALGORITHM_THREE_WAY_COMP_REF_TYPE_H
 #define _LIBCPP___ALGORITHM_THREE_WAY_COMP_REF_TYPE_H
 
 #include <__compare/ordering.h>
 #include <__config>
 #include <__debug>
 #include <__utility/declval.h>
+#include <__utility/forward.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #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()(_Tp& __x, _Up& __y) {
-    auto __r = __comp_(__x, __y);
+  _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __x, _Up&& __y) {
+    auto __r = __comp_(std::forward<_Tp>(__x), std::forward<_Up>(__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

 
 // template<class InputIterator1, class InputIterator2, class Cmp>
 //     constexpr auto
 //     lexicographical_compare_three_way(InputIterator1 first1, InputIterator1 last1,
 //                                       InputIterator2 first2, InputIterator2 last2,
 //                                       Cmp comp)
 //       -> decltype(comp(*b1, *b2));
 
-#include <array>
 #include <algorithm>
+#include <array>
 #include <cassert>
 #include <compare>
 #include <concepts>
 #include <limits>
 
-#include "test_macros.h"
 #include "test_iterators.h"
+#include "test_macros.h"
 
 using std::array;
 
 constexpr auto compare_last_digit_strong  = [](int a, int b) -> std::strong_ordering { return (a % 10) <=> (b % 10); };
 constexpr auto compare_last_digit_weak    = [](int a, int b) -> std::weak_ordering { return (a % 10) <=> (b % 10); };
 constexpr auto compare_last_digit_partial = [](int a, int b) -> std::partial_ordering {
   if (a == std::numeric_limits<int>::min() || b == std::numeric_limits<int>::min())
     return std::partial_ordering::unordered;
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);
+  int expected_compare_invocation_count = 3;
+#ifdef _LIBCPP_ENABLE_DEBUG_MODE
+  if (!std::is_constant_evaluated()) {
+    // In debug mode the comparator will be called twice by the debugging wrapper `__debug_three_way_comp`
+    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;
 }