[libcxx-commits] [PATCH] D122780: Modify std::sort: add BlockQuickSort partitioning algorithm for arithmetic types

[Nilay Vaish via Phabricator via libcxx-commits]

nilayvaish added inline comments.


================
Comment at: libcxx/include/__algorithm/sort.h:234
 template <class _Compare, class _RandomAccessIterator>
 void __insertion_sort_3(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp) {
   typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
----------------
philnik wrote:
> Why is this named `_3`? That's quite confusing.
Dropped _3.  This was named so likely because it was using __sort3 for the first three elements.  I have dropped that code.


================
Comment at: libcxx/include/__algorithm/sort.h:268
+    if (__comp(*__i, *__j)) {
+      value_type __t(_VSTD::move(*__i));
+      _RandomAccessIterator __k = __j;
----------------
philnik wrote:
> You can use `std::` in new code. Applies throughout the patch.
Changed _VSTD to std throughout the file.


================
Comment at: libcxx/include/__algorithm/sort.h:660-662
+  _LIBCPP_CONSTEXPR_AFTER_CXX11 difference_type __limit = 24;
+  // Lower bound for using Tuckey's ninther technique for median computation.
+  _LIBCPP_CONSTEXPR_AFTER_CXX11 difference_type __ninther_threshold = 128;
----------------
philnik wrote:
> Why can't these be `_LIBCPP_CONSTEXPR`?
Fixed.


================
Comment at: libcxx/include/__algorithm/sort.h:685-686
     }
-    if (__len <= __limit) {
-      _VSTD::__insertion_sort_3<_Compare>(__first, __last, __comp);
+    // Use insertion sort if the length of the range is below the specified
+    // limit.
+    if (__len < __limit) {
----------------
philnik wrote:
> 
I have fixed it.  But clang-format might reformat it.


================
Comment at: libcxx/include/__algorithm/sort.h:750-751
     }
-    // sort smaller range with recursive call and larger with tail recursion elimination
-    if (__i - __first < __last - __i) {
-      _VSTD::__introsort<_Compare>(__first, __i, __comp, __depth);
-      __first = ++__i;
-    } else {
-      _VSTD::__introsort<_Compare>(__i + difference_type(1), __last, __comp, __depth);
-      __last = __i;
-    }
+    // Sort the left partiton recursively and the right partition with tail
+    // recursion elimination.
+    _VSTD::__introsort<_Compare, _RandomAccessIterator, _UseBitSetPartition>(__first, __i, __comp, __depth, __leftmost);
----------------
philnik wrote:
> 
Same comment as above.


================
Comment at: libcxx/include/__algorithm/sort.h:772-774
+  // Only use bitset partitioning for arithmetic types.  We should also check
+  // that the default comparator is in use so that we are sure that there are no
+  // branches in the comparator.
----------------
philnik wrote:
> Where do you check that the default comparator is used?
Fixed.  Actually I was not aware how to test for the default comparator.   I have now switched to using __use_branchless_sort declaration that is used at other places in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122780