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

[Nikolas Klauser via Phabricator via libcxx-commits]

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


================
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,
----------------
Could you do the whitespace formatting in a different patch?


================
Comment at: libcxx/include/__algorithm/ranges_nth_element.h:46
+
+    auto&& __projected_comp = __make_projected_comp(__comp, __proj);
+    std::__nth_element_impl(std::move(__first), std::move(__nth), __last_iter, __projected_comp);
----------------



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


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.nth.element/ranges_nth_element.pass.cpp:146
+
+  { // 3-element sequence.
+    std::array input = {2, 1};
----------------



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.nth.element/ranges_nth_element.pass.cpp:195
+
+  { // Reverse sorted.
+    std::array input = {6, 5, 4, 3, 2, 1};
----------------
Descending?


================
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;
+    };
----------------
I don't think this should be here.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128149