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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: ldionne.
ldionne added a comment.

All in all, LGTM with some comments.

You should poke the CI.



================
Comment at: libcxx/include/__algorithm/nth_element.h:31
 template<class _Compare, class _RandomAccessIterator>
-_LIBCPP_CONSTEXPR_AFTER_CXX11 bool
-__nth_element_find_guard(_RandomAccessIterator& __i, _RandomAccessIterator& __j,
----------------
philnik wrote:
> Could you do the whitespace formatting in a different patch?
+1, it'll make it possible to ignore it for `git blame`. We should really strive to do these mostly-whitespace changes in NFC patches, otherwise we'll destroy our ability to `git blame`, and we'll pay for that in the future.


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


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


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.nth.element/ranges_nth_element.pass.cpp:214
+
+constexpr void test_iterators() {
+  test_iterators_1<random_access_iterator<int*>>();
----------------
Instead of having multiple `test_iterator_n` functions, I would do something like this (IMO it localizes things which makes it more obvious where they are used):

```
constexpr void test_iterators() {
  auto check = []<class Iter, class Sent> {
    // all checks
  };

  check.operator()<random_access_iterator<int*>, random_access_iterator<int*>>();
  check.operator()<random_access_iterator<int*>, sentinel_wrapper<random_access_iterator<int*>>>();

  // etc.
}
```

If you dislike the lambda, you could define it as a function template named e.g. `test_iterators_impl`, but the point here is to make it obvious that it's only an implementation detail of `test_iterators`. Otherwise, I'd have to check whether `test_iterators_1` is somehow called from elsewhere in the file, and whether that might be the reason for giving it a "global" name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128149