[libcxx-commits] [PATCH] D131395: [libc++] Implement `lexicographical_compare_three_way`

Thu Dec 8 22:46:52 PST 2022

var-const added inline comments.

================
Comment at: libcxx/benchmarks/lexicographical_compare_three_way.bench.cpp:28
+  for (auto _ : state) {
+    benchmark::DoNotOptimize(std::__lexicographical_compare_three_way_slow_path(b1, e1, b2, e2));
+  }
----------------
Hmm, I think the `slow/fast_path` functions still need the comparator passed explicitly.

================
Comment at: libcxx/benchmarks/lexicographical_compare_three_way.bench.cpp:73
+  for (auto _ : state) {
+    benchmark::DoNotOptimize(std::lexicographical_compare_three_way(b1, e1, b2, e2, std::compare_three_way()));
+  }
----------------
avogelsgesang wrote:
> var-const wrote:
> > var-const wrote:
> > > Nit: passing this is unnecessary because it's the default value, right?
> > Nit: move?
> > Nit: passing this is unnecessary because it's the default value, right?
> 
> correct. removed
> 
> > Nit: move?
> 
> move what? the input iterators? I don't think that is possible without a "use-after-move". I need them for each iteration of the `for` loop.
Right, please disregard (I might have been looking at a previous iteration of this code where iterators were initialized inside the loop).

================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:109
+    _InputIterator1 __first1, _InputIterator1 __last1, _InputIterator2 __first2, _InputIterator2 __last2) {
+  return std::lexicographical_compare_three_way(__first1, __last1, __first2, __last2, compare_three_way());
+}
----------------
avogelsgesang wrote:
> var-const wrote:
> > Move the iterators? (in the other function as well)
> > 
> > You might additionally `static_assert` that the given iterators are copyable, to prevent users from accidentally passing move-only iterators (that our implementation would happen to accept due to the optimization but which isn't guaranteed by the Standard). A few of the existing algorithms do that, see e.g. `upper_bound`.
> Is adding the additional calls to `std::move` standard compliant? https://eel.is/c++draft/alg.three.way defines `lexicographical_compare_three_way` as not moving the iterators
I think we are allowed to do that (as long as the difference is not observable), and it is done in a few places. I can't think of a valid way a user could observe the difference -- since we are allowed to copy or move the iterators in the implementation, they can't rely on move constructor not being called or a copy constructor only being called once.

================
Comment at: libcxx/include/__algorithm/three_way_comp_ref_type.h:9
+
+#ifndef _LIBCPP___ALGORITHM_THREE_WAY_COMP_REF_TYPE_H
+#define _LIBCPP___ALGORITHM_THREE_WAY_COMP_REF_TYPE_H
----------------
You might need to run something like `ninja -C build libcxx-generate-files` to take the new header into account.

================
Comment at: libcxx/include/__algorithm/three_way_comp_ref_type.h:27
+  _Comp& __comp_;
+  _LIBCPP_CONSTEXPR_SINCE_CXX14 __debug_three_way_comp(_Comp& __c) : __comp_(__c) {}
+
----------------
Functions in this file should be marked `_LIBCPP_HIDE_FROM_ABI`.

================
Comment at: libcxx/include/__algorithm/three_way_comp_ref_type.h:30
+  template <class _Tp, class _Up>
+  constexpr decltype(auto) operator()(_Tp& __x, _Up& __y) {
+    auto __r = __comp_(__x, __y);
----------------
Hmm, looks like the return type can be just `auto`? (if I'm reading this correctly, it can never return a reference)

================
Comment at: libcxx/include/__algorithm/three_way_comp_ref_type.h:37
+  template <class _LHS, class _RHS, class _Order>
+  _LIBCPP_CONSTEXPR_SINCE_CXX14 inline _LIBCPP_INLINE_VISIBILITY void
+  __do_compare_assert(int, _LHS& __l, _RHS& __r, _Order __o)
----------------
Note: `_LIBCPP_INLINE_VISIBILITY` was used in old code, in new code we use `_LIBCPP_HIDE_FROM_ABI` for this (they do the same thing, though).

================
Comment at: libcxx/include/__algorithm/three_way_comp_ref_type.h:39
+  __do_compare_assert(int, _LHS& __l, _RHS& __r, _Order __o)
+    requires __comparison_category<decltype(declval<_Comp&>()(declval<_LHS&>(), declval<_RHS&>()))>
+  {
----------------
Question: do we need this `requires` clause? We only use the class in our internal code, so it seems like we shouldn't need this. If it's to validate the comparator given by the user, then it shouldn't only be done in the debug mode.

================
Comment at: libcxx/include/__algorithm/three_way_comp_ref_type.h:46
+      __expected = _Order::less;
+    _LIBCPP_DEBUG_ASSERT(__comp_(__l, __r) != __expected, "Comparator does not induce a strict weak ordering");
+    (void)__l;
----------------
Hmm, shouldn't this be `== __expected`? IIUC, `__expected` is already "reversed".

================
Comment at: libcxx/include/__algorithm/three_way_comp_ref_type.h:53
+  template <class _LHS, class _RHS, class _Order>
+  _LIBCPP_CONSTEXPR_SINCE_CXX14 inline _LIBCPP_INLINE_VISIBILITY void __do_compare_assert(long, _LHS&, _RHS&, _Order) {}
+};
----------------
Since this class's definition is only available in C++20 and above, this can be just `constexpr` (throughout the file).

================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:3
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
avogelsgesang wrote:
> var-const wrote:
> > We have a few tests that check for a certain behavior across a wide range of algorithms; those have to be updated to include `lexicographical_compare_three_way` now:
> > - `test/libcxx/diagnostics/nodiscard_extensions.verify.cpp` (potentially);
> > - `test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp`;
> > - `test/libcxx/algorithms/robust_against_cpp20_hostile_iterators.compile.pass.cpp`;
> > - `test/std/algorithms/robust_re_difference_type.compile.pass.cpp`;
> > - `test/std/algorithms/robust_against_adl.compile.pass.cpp`.
> > 
> > Please let me know if you need any help with those.
> I added `lexicographical_compare_three_way` to all of them, except for `robust_re_difference_type.compile.pass.cpp`.
> Afaict this test case relies on undefined behavior. The `difference_type` must be an integer type (https://eel.is/c++draft/iterator.iterators#2.2), but this test case violates that requirement.
> As requested by @philnik I added a `static_assert` to `lexicographical_compare_three_way` which guards against non-integer different_types. Hence, adding `lexicographical_compare_three_way` triggers this static_assert.
> I see two ways forward: Not adding `lexicographical_compare_three_way` to `robust_re_difference_type.compile.pass.cpp` or removing the `static_assert`. Which one do you prefer?
> * 
Hmm, it's a great question. I'm not sure why this wasn't just `static_assert`ed before. I'll check -- in the meantime, I think it's best to leave the `static_assert` in place and add a comment to the test file to explain that `lexicographical_compare_three_way` is omitted intentionally.

================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:109
     // RELIES ON ADL SWAP (void)std::iter_swap(first, mid);
-    (void)std::lexicographical_compare(first, last, first2, last2);
+    //(void)std::lexicographical_compare(first, last, first2, last2);
     (void)std::lexicographical_compare(first, last, first2, last2, std::less<void*>());
----------------
This looks like it was commented out accidentally?

================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:112
+#if TEST_STD_VER > 17
+    //(void)std::lexicographical_compare_three_way(first, last, first2, last2);
+    (void)std::lexicographical_compare_three_way(first, last, first2, last2, std::compare_three_way());
----------------
Can this be uncommented?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395