[libcxx-commits] [PATCH] D141614: [libc++] nth_element change to partial sort when range is really small

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for your contribution!

Can you show the benchmark results of the improvement, if needed by adding a new benchmark.



================
Comment at: libcxx/include/__algorithm/nth_element.h:29
 
+// Assumes size > 0
+template <class _AlgPolicy, class _Compare, class _BidirectionalIterator>
----------------
Please make this a `_LIBCPP_ASSERT` instead.


================
Comment at: libcxx/include/__algorithm/nth_element.h:30
+// Assumes size > 0
+template <class _AlgPolicy, class _Compare, class _BidirectionalIterator>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void __selection_partial_sort(
----------------
I really love to have some documentation for this function.


================
Comment at: libcxx/include/__algorithm/nth_element.h:33
+    _BidirectionalIterator __first, _BidirectionalIterator __nth, _BidirectionalIterator __last, _Compare __comp) {
+  _BidirectionalIterator __lm1 = __nth;
+  if (++__lm1 == __last)
----------------
Why not use __nth? The new variable name does not help me what this does.


================
Comment at: libcxx/include/__algorithm/nth_element.h:34
+  _BidirectionalIterator __lm1 = __nth;
+  if (++__lm1 == __last)
+    --__lm1;
----------------
If `__nth == __last` this will fail.


================
Comment at: libcxx/include/__algorithm/nth_element.h:37
+  for (; __first != __lm1; ++__first) {
+    _BidirectionalIterator __i = std::__min_element<_Compare>(__first, __last, __comp);
+    if (__i != __first)
----------------
Is `_Compare` not deduced?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141614