[libcxx-commits] [PATCH] D141205: [libcxx] nth_element use introselect to avoid quadratic behavior

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__algorithm/nth_element.h:52
+
+    typename iterator_traits<_RandomAccessIterator>::difference_type __len = __middle - __first;
+    _RandomAccessIterator __i = __middle;
----------------
perhaps use `typename _IterOps<_AlgPolicy>::__diference_type`


================
Comment at: libcxx/include/__algorithm/nth_element.h:104
+        {
+            // fall back to heap select.
+            std::__heap_select<_AlgPolicy, _Compare>(__first, __nth, __last, __comp);
----------------
danlark wrote:
> Backl1ght wrote:
> > danlark wrote:
> > > Mordante wrote:
> > > > Can you add comment why this is done? Currently the comment and the code tell the same thing.
> > > > For maintainability it would be very good to know why we switch the algorithm.
> > > Even though I am in favor of this patch, this does not fully fix the issue. With the comparator that changes the inputs on the fly, it's possible to get into a O(n log n) average case.
> > > 
> > > Otherwise LGTM, thanks!
> > I think the average time complexity of introselect should be O(n), since the average time complexity of quickselect is O(n), and according to the original paper of introselect:
> > 
> > > If as in introsort we limit the depth to a logarithmic bound the average time remains linear but the worst case is O(N log N). 
> The example from the link https://github.com/llvm/llvm-project/issues/52747#issuecomment-996916909 will still be of average case O(N log N)
I think that example should be UB. I can't remember the legacy version's wording, but I am pretty sure the ranges version requires the `predicate` to be "equality-preserving". The same inputs  to the predicate should return the same output. But that example's predicate seems to modify the states in such way that same inputs can result in different output 


================
Comment at: libcxx/include/__algorithm/nth_element.h:105
+            // fall back to heap select.
+            std::__heap_select<_AlgPolicy, _Compare>(__first, __nth, __last, __comp);
+            return;
----------------
nit:  `_Compare` can be deduced


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

https://reviews.llvm.org/D141205