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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this!



================
Comment at: libcxx/benchmarks/lexicographical_compare_three_way.bench.cpp:34
+// `lexicographical_compare_three_way` has a fast path for random access iterators.
+// `int*` is a random access iterator whie `cpp17_input_iterator` is not.
+BENCHMARK_TEMPLATE(BM_lexicographical_compare_three_way, int*)->RangeMultiplier(2)->Range(1, 1 << 20);
----------------



================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:44-45
+                  "A non-integral difference_type is undefined behavior");
+    static_assert(is_integral_v<typename iterator_traits<_InputIterator2>::difference_type>,
+                  "A non-integral difference_type is undefined behavior");
+    auto __len1    = __last1 - __first1;
----------------
At Discord we discovered concepts give nicer error messages. Can you test whether that's true here too, by using `integral` instead of `is_integral_v`?

Since you use two asserts, lets give additional information; alternatively merge them in one assert.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:48
+    auto __len2    = __last2 - __first2;
+    auto __min_len = __len1 < __len2 ? __len1 : __len2;
+    for (decltype(__min_len) __i = 0; __i < __min_len;) {
----------------
Why not using `std::min` here? I assume it is because the types of `__len1` and `__len2` can differ.

I'm not happy with all the `auto`s in this code; it's hard to understand what the intended types are and whether these are the correct intended types. Please remove the `auto`s so it's clear what the types are.

I really would like something along the lines of
```
__iter_diff_t<_InputIterator1> __len1 = __last1 - __first1;
__iter_diff_t<_InputIterator2> __len2 = __last2 - __first2;
auto __min = std::min<common_type_t<decltype(__len1), decltype(__len2)>(__len1, __len2));
```
This is more verbose, but at least I can understand how the types are defined and whether this is as intended.



================
Comment at: libcxx/include/algorithm:1697
+    lexicographical_compare_three_way(InputIterator1 first1, InputIterator1 last1,
+                                      InputIterator2 first2, InputIterator2 last2);              // since C++20
+
----------------
Can you align the version number comments?


================
Comment at: libcxx/include/module.modulemap.in:291
       module lexicographical_compare         { private header "__algorithm/lexicographical_compare.h" }
+      module lexicographical_compare_three_way { private header "__algorithm/lexicographical_compare_three_way.h" }
       module lower_bound                     { private header "__algorithm/lower_bound.h" }
----------------
Can you indent the other `{`s to the same level.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:27
+
+using std::array;
+
----------------
Please use the qualified name in the tests.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:33
+      std::lexicographical_compare_three_way(Iter1{a.begin()}, Iter1{a.end()}, Iter2{b.begin()}, Iter2{b.end()});
+  assert(expected == result);
+}
----------------
I don't mind this way of testing, but since the function is expressed in terms of the version with a `compare_three_way` it would have been possible to test this version against that version.
Something along the lines of:

```
auto expected = std::lexicographical_compare_three_way(Iter1{a.begin()}, Iter1{a.end()}, Iter2{b.begin()}, Iter2{b.end()}, std::compare_three_way());
std::same_as<decltype(expected)> auto result = std::lexicographical_compare_three_way(Iter1{a.begin()}, Iter1{a.end()}, Iter2{b.begin()}, Iter2{b.end()});

assert(expected == result);
```
That way we might even remove some tests here and relay on the other test to provide the coverage.



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.verify.cpp:18
+//       -> decltype(comp(*b1, *b2));
+
+#include <array>
----------------
Can you also test with iterators with invalid difference types?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.verify.cpp:28
+
+int main(int, char**) {
+  std::array a{90, 81};
----------------
In general we don't use `main` in our verify test, since the code isn't executed.


================
Comment at: libcxx/test/support/test_comparisons.h:243
+#if TEST_STD_VER > 17
+struct WeakComp {
+  int value;
----------------
(Normally I wouldn't nitpick on naming, but the comment at the top of this header lists its conventions.)

In this header Comp/Comparison refers to C++17. Please rename to `WeakOrder`.



================
Comment at: libcxx/test/support/test_comparisons.h:253
+  friend std::partial_ordering operator<=>(PartialComp, PartialComp) = default;
+};
+#endif
----------------
Why not complete the set with `std::strong_ordering`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395