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

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added inline comments.


================
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()));
+  }
----------------
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.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:66
+_LIBCPP_HIDE_FROM_ABI constexpr auto __lexicographical_compare_three_way_slow_path(
+    _InputIterator1 __first1, _InputIterator1 __last1, _InputIterator2 __first2, _InputIterator2 __last2, _Cmp __comp)
+    -> decltype(__comp(*__first1, *__first2)) {
----------------
var-const wrote:
> Would it make sense to pass `__comp` by reference? It could, in theory, be e.g. a lambda that captures a lot of state. In fact, we generally commit to avoid copying user-provided functors. However, see also the other comment about `__comp_ref_type` (which would make this a reference but also have an additional effect in the debug mode).
passing as `_Cmp&` now


================
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());
+}
----------------
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


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:100
+    return __lexicographical_compare_three_way_fast_path(
+        __first1, __last1, __first2, __last2, std::forward<_Cmp>(__comp));
+  } else {
----------------
var-const wrote:
> philnik wrote:
> > var-const wrote:
> > > JohelEGP wrote:
> > > > avogelsgesang wrote:
> > > > > Mordante wrote:
> > > > > > Is `forward` intended? `__comp` is passed by value.
> > > > > good catch! Changed to `move`
> > > > I think `ref(__comp)` is the preferred way.
> > > @JohenEGP I presume you referring to the `__comp_ref_type`, right? It's a great suggestion.
> > > 
> > > @avogelsgesang For context, there is an existing pattern in algorithms where the internal implementation of the function wraps the comparator in a typedef that is defined differently in debug and non-debug modes (see `include/__algorithm/comp_ref_type.h`):
> > > ```
> > > #ifdef _LIBCPP_ENABLE_DEBUG_MODE
> > > template <class _Comp>
> > > using __comp_ref_type = __debug_less<_Comp>;
> > > #else
> > > template <class _Comp>
> > > using __comp_ref_type = _Comp&;
> > > #endif
> > > ```
> > > 
> > > So in non-debug modes, this resolves to simply a reference. In debug mode, however, it creates a temporary of a helper type `__debug_less` that additionally checks that the comparator in fact does induce a strict weak order.
> > > 
> > > I think we want to continue using this pattern going forward (e.g. `lexicographical_compare` uses it). The only issue is that the existing `__debug_less` only supports comparators returning a boolean. You would probably need to create a separate `__three_way_comp_ref_type` typedef and a separate `__debug_three_way_comp` helper struct (names are subject to change).
> > > 
> > > Please let me know if you'd like any additional context on this (this can be kinda confusing). Many existing algorithms can be used as examples of this pattern; a caveat is that more recent code uses a (very) slightly different approach:
> > > - older code tends to call have `_Compare` as a template parameter of the internal function, declare the parameter as `_Compare __comp`, and have the caller specify the template argument explicitly, like `std::__foo<_comp_ref_type<_Comp>>(__first, __last, __comp)`. See e.g. `lexicographical_compare`;
> > > - in newer code, the pattern was to declare the parameter as a forwarding reference `_Compare&& __comp` and have the caller do a `static_cast`, like `std::__foo(__first, __last, static_cast<__comp_ref_type(__comp)>` (see e.g. `inplace_merge`).
> > > 
> > > If you prefer to, I'm fine with doing this in a follow up since this patch has been open for a while now.
> > IMO we should make the debug stuff a separate effort. AFAIK we don't test it anywhere and because of that I'm pretty sure it regressed in some places. Instead of adding another untested branch, I'd suggest creating a patch that adds tests to all algorithms and, depending on scope, update the algorithms in follow-up patches or as part of the test-patch.
> I would agree to that if someone volunteers to do that follow-up in the near future -- in fact, it would be great. Would you or @avogelsgesang be willing to take this on?
added the `__three_way_comp_ref_type` as requested


================
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.
----------------
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?
* 


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp:31
+
+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); };
----------------
var-const wrote:
> Question: what is the purpose of comparing just the last digit instead of just the two given numbers? It makes the implementation slightly more complicated (and the variable names longer), but all the inputs are single-digit anyway.
the idea is to test a "non-default" comparator. If I just used normal integer comparisons here, the test cases wouldn't catch it if `lexicographical_compare_three_way` just ignored the comparator and used the standard comparator instead


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp:34
+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;
----------------
var-const wrote:
> Is this branch ever taken?
see "Check for a `partial_ordering::unordered` result" inside `test_comparison_categories`


================
Comment at: libcxx/test/support/test_comparisons.h:263
+        rhs.value == std::numeric_limits<int>::min())
+      return std::partial_ordering::unordered;
+    return lhs.value <=> rhs.value;
----------------
var-const wrote:
> Is this branch ever taken?
See "Check for a `partial_ordering::unordered` result" in `lexicographical_compare_three_way.pass.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395