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

[Hui via Phabricator via libcxx-commits]

huixie90 requested changes to this revision.
huixie90 added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:24
+
+template <class InputIterator1, class InputIterator2, class Cmp>
+_LIBCPP_HIDE_FROM_ABI constexpr auto lexicographical_compare_three_way(
----------------



================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:51
+
+template <class InputIterator1, class InputIterator2>
+_LIBCPP_HIDE_FROM_ABI constexpr auto lexicographical_compare_three_way(
----------------
We need to uglify these type names


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:33
+
+// A struct for which only weak comparisons are defined
+struct PartialInt {
----------------
wrong comments?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:48
+template <typename Iter1, typename Iter2>
+constexpr void test_iterator_types() {
+  // Identical arrays
----------------
please also add tests for cases where either range is empty or both of them are empty


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:48
+template <typename Iter1, typename Iter2>
+constexpr void test_iterator_types() {
+  // Identical arrays
----------------
huixie90 wrote:
> please also add tests for cases where either range is empty or both of them are empty
Could you please also add a test to test
"Complexity: At most N applications of comp."


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:51-52
+  assert((test_lexicographical_compare<Iter1, Iter2>(array{0, 1}, array{0, 1}, std::strong_ordering::equal)));
+  // "Greater" on 2nd element
+  assert((test_lexicographical_compare<Iter1, Iter2>(array{0, 1}, array{0, 2}, std::strong_ordering::less)));
+  // "Less" on 2nd element
----------------
the comment is a bit ambiguous. which range's 2nd element is "greater"?. Since the result is "less". the comment "greater" is slightly counter-intuitive


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:62
+}
+
+constexpr bool test() {
----------------
The spec has explicitly specifies the return type ` -> decltype(__comp(*__first1, *__first2))` and this has a SFINAE effect.
It would be good to test the SFINAE effect as well (if __comp is not callbale then the function should be SFINAEed out)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:70-71
+  // Check for other comparison categories
+  assert((test_lexicographical_compare<const WeakInt*, const WeakInt*>(
+      array<WeakInt, 2>{{{0}, {1}}}, array<WeakInt, 2>{{{1}, {1}}}, std::weak_ordering::less)));
+  assert((test_lexicographical_compare<const PartialInt*, const PartialInt*>(
----------------
Could you please at least add a test for the weak_ordering where one range is shorter than the other and all the elements are equal. IIUC, in this case, the result is going to convert from a strong_ordering to a weak_ordering.




================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp:71
+  // Check with various iterator types
+  test_iterator_types<const int*, const int*>();
+  test_iterator_types<const int*, forward_iterator<const int*>>();
----------------
Please add a test where the input iterator only models c++17's `input_iterator` (I think you can use `cpp17_input_iterator`. Since the InputIterator is the minimum requirement


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395