[libcxx-commits] [PATCH] D128149: [libc++][ranges] Implement `ranges::nth_element`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/nth_element.h:71
+          _RandomAccessIterator __m = __first;
+          std::__sort3<_Compare>(__first, ++__m, --__last, __comp);
+          return;
----------------
huixie90 wrote:
> similar for other reiviews, `ranges` overload require the implementation to use `iter_move` and `iter_swap`
Thanks a lot for finding this issue! Will fix in a follow-up.


================
Comment at: libcxx/include/__algorithm/nth_element.h:97
+      if (std::__nth_element_find_guard<_Compare>(__i, __j, __m, __comp)) {
+        swap(*__i, *__j);
+        ++__n_swaps;
----------------
huixie90 wrote:
> `ranges` overload needs to use `iter_swap`. same for the rest of swaps
Same as the other comment -- will fix in a follow-up.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.nth.element/ranges_nth_element.pass.cpp:72
+  // check for specific output in the libc++ implementation.
+  LIBCPP_ASSERT(partially_sorted == expected);
+  assert(base(last) == partially_sorted.end());
----------------
ldionne wrote:
> philnik wrote:
> > I don't think we should do that. It's quite fragile and we even have `_LIBCPP_DEBUG_RANDOMIZE_RANGE` to avoid that users depend on the exact output.
> I agree. I think this is going to be brittle if we make changes to our algorithms. Also, if you remove this, you can (and should) still check that the value of the nth element is as expected (i.e. instead of passing an expected array, you can pass an expected value (or iterator to validate the position)).
Thanks, this makes the tests way less verbose. Rather than passing values explicitly, it seems easier to create a copy of the input and sort it, then compare the `n`th elements between the partially-sorted and fully-sorted sequences.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.nth.element/ranges_nth_element.pass.cpp:164-168
+    std::array<decltype(input), input.size()> expected_array = {
+      expected1, expected1, expected1, expected1, expected2, expected3, expected3, expected3
+    };
+
+    test_all_cases<Iter, Sent>(input, expected_array);
----------------
ldionne wrote:
> IMO this is a bit complicated, we could instead just manually call `test_all_cases` with each `(input, n, expected)` combination. And that would allow getting rid of the `array<array<...>>` overload of `test_all_cases`, which I had to pause quite a bit on.
(no longer applies now that we're not checking for the exact output)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.nth.element/ranges_nth_element.pass.cpp:246
+      int a;
+      constexpr bool operator==(const A&) const = default;
+    };
----------------
philnik wrote:
> I don't think this should be here.
Same as the other review -- this is necessary to compare the value of the `n`th element.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.nth.element/ranges_nth_element.pass.cpp:275
+
+      constexpr bool operator==(const S&) const = default;
+    };
----------------
philnik wrote:
> Also this.
(see the other comment)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128149