[libcxx-commits] [PATCH] D118003: [libc++] Floyd's improvement to pop_heap

[Nikolas Klauser via Phabricator via libcxx-commits]

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

LGTM % nits, but I'm not exactly an expert here.



================
Comment at: libcxx/include/__algorithm/pop_heap.h:32
 {
+    typedef typename iterator_traits<_RandomAccessIterator>::value_type value_type;
+
----------------



================
Comment at: libcxx/include/__algorithm/pop_heap.h:34-47
     if (__len > 1)
     {
-        swap(*__first, *--__last);
-        _VSTD::__sift_down<_Compare>(__first, __comp, __len - 1, __first);
+        value_type __top = _VSTD::move(*__first);  // create a hole at __first
+        _RandomAccessIterator __hole = _VSTD::__floyd_sift_down<_Compare>(__first, __comp, __len);
+        --__last;
+        if (__hole == __last) {
+            *__hole = _VSTD::move(__top);
----------------
I'd rather see an early return, but I may be alone.


================
Comment at: libcxx/include/__algorithm/sift_down.h:78
+template <class _Compare, class _RandomAccessIterator>
+_LIBCPP_CONSTEXPR_AFTER_CXX11 _RandomAccessIterator
+__floyd_sift_down(_RandomAccessIterator __first, _Compare __comp,
----------------
Is `_LIBCPP_HIDE_FROM_ABI` purposefully not here?


================
Comment at: libcxx/include/__algorithm/sift_down.h:82
+{
+    typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
+    _LIBCPP_ASSERT(__len >= 2, "shouldn't be called unless __len >= 2");
----------------



================
Comment at: libcxx/include/__algorithm/sift_down.h:104-106
+        if (__child > (__len - 2) / 2) {
+            return __hole;
+        }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118003