[libcxx-commits] [PATCH] D128115: [libc++][ranges] Implement modifying heap algorithms:

[Hui via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__algorithm/make_heap.h:33
+    for (difference_type __start = (__n - 2) / 2; __start >= 0; --__start) {
+        std::__sift_down<_Compare>(__first, __comp, __n, __first + __start);
     }
----------------
I think `__sift_down` does not use `iter_move` or `iter_swap`. This is not correct for `ranges` algorithms


================
Comment at: libcxx/include/__algorithm/pop_heap.h:33
+  if (__len > 1) {
+    value_type __top = std::move(*__first);  // create a hole at __first
+    _RandomAccessIterator __hole = std::__floyd_sift_down<_Compare>(__first, __comp, __len);
----------------
I believe this needs to call `ranges::iter_move` for the ranges version


================
Comment at: libcxx/include/__algorithm/pop_heap.h:40
+    } else {
+      *__hole = std::move(*__last);
+      ++__hole;
----------------
same as above


================
Comment at: libcxx/include/__algorithm/pop_heap.h:43
+      *__last = std::move(__top);
+      std::__sift_up<_Compare>(__first, __hole, __comp, __hole - __first);
     }
----------------
I think this does not do the right thing for ranges overload


================
Comment at: libcxx/include/__algorithm/push_heap.h:35-37
+      value_type __t(std::move(*__last));
+      do {
+        *__last = std::move(*__ptr);
----------------
should use iter_move for `ranges` overload


================
Comment at: libcxx/include/__algorithm/sort_heap.h:31
+  for (difference_type __n = __last - __first; __n > 1; --__last, (void) --__n)
+    std::__pop_heap<_Compare>(__first, __last, __comp, __n);
 }
----------------
This won't call the `ranges::iter_move` for the ranges overload


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128115