[libcxx-commits] [PATCH] D150188: [libc++][spaceship] Fixed `__debug_three_way_comp`'s `operator()`

[Hristo Hristov via Phabricator via libcxx-commits]

H-G-Hristov created this revision.
Herald added a subscriber: yaxunl.
Herald added a project: All.
H-G-Hristov retitled this revision from "[libc++][spaceship] Fixed `three_way_comp_ref_type`'s `operator()`" to "[libc++][spaceship] Fixed `__debug_three_way_comp`'s `operator()`".
H-G-Hristov edited the summary of this revision.
H-G-Hristov edited the summary of this revision.
H-G-Hristov updated this revision to Diff 520717.
H-G-Hristov added a comment.
H-G-Hristov updated this revision to Diff 520816.
H-G-Hristov updated this revision to Diff 520908.
H-G-Hristov updated this revision to Diff 520939.
H-G-Hristov updated this revision to Diff 521418.
H-G-Hristov updated this revision to Diff 521564.
H-G-Hristov updated this revision to Diff 521966.
H-G-Hristov published this revision for review.
H-G-Hristov added a subscriber: philnik.
Herald added a reviewer: jdoerfert.
Herald added subscribers: libcxx-commits, jplehr, sstefan1.
Herald added a project: libc++.
Herald added a reviewer: libc++.

Try to fix CI


H-G-Hristov added a comment.

Try to fix CI


H-G-Hristov added a comment.

Try to fix CI


H-G-Hristov added a comment.

Try to fix CI


H-G-Hristov added a comment.

Try to fix CI


H-G-Hristov added a comment.

Try to fix CI


H-G-Hristov added a comment.

- Use "perfect forwarding"


H-G-Hristov added a comment.

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


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

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

`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.

This patch aims to resolve the issues.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150188

Files:
  libcxx/include/__algorithm/three_way_comp_ref_type.h
  libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp


Index: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp
===================================================================
--- libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp
+++ libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp
@@ -16,15 +16,15 @@
 //                                       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;
 
@@ -155,7 +155,14 @@
   // 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() {
Index: libcxx/include/__algorithm/three_way_comp_ref_type.h
===================================================================
--- libcxx/include/__algorithm/three_way_comp_ref_type.h
+++ libcxx/include/__algorithm/three_way_comp_ref_type.h
@@ -13,6 +13,7 @@
 #include <__config>
 #include <__debug>
 #include <__utility/declval.h>
+#include <__utility/forward.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
@@ -28,8 +29,8 @@
   _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;
   }
@@ -55,7 +56,7 @@
 
 // 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D150188.521966.patch
Type: text/x-patch
Size: 2661 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230514/948c41d0/attachment-0001.bin>