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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good, but there are some small issues.



================
Comment at: libcxx/include/__algorithm/make_heap.h:42
 template <class _RandomAccessIterator, class _Compare>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
-void
-make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp)
-{
-    typedef typename __comp_ref_type<_Compare>::type _Comp_ref;
-    _VSTD::__make_heap<_Comp_ref>(__first, __last, __comp);
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+void make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp) {
----------------
Maybe drop the `inline` while you're at it.


================
Comment at: libcxx/include/__algorithm/push_heap.h:39
+        __last = __ptr;
+        if (__len == 0) break;
+        __len = (__len - 1) / 2;
----------------
Can you put the `break;` on its own line?


================
Comment at: libcxx/include/__algorithm/push_heap.h:53
+  using _CompRef = typename __comp_ref_type<_Compare>::type;
+  std::__sift_up<_CompRef>(std::move(__first), std::move(__last), __comp, __last - __first);
 }
----------------
You need to calculate `__last - __first` before the function call since they are moved from in this call.


================
Comment at: libcxx/include/__algorithm/ranges_pop_heap.h:48
+    auto&& __projected_comp = ranges::__make_projected_comp(__comp, __proj);
+    std::__pop_heap(std::move(__first), __last_iter, __projected_comp, __last_iter - __first);
+
----------------
Here `__first` might be used in a moved from state.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp:43
+
+static_assert(HasMakeHeapIt<int*>);
+static_assert(!HasMakeHeapIt<RandomAccessIteratorNotDerivedFrom>);
----------------
I think it would be good to test with all iterator categories to see only random access and contiguous are valid.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp:217
+  return true;
+}
+
----------------
I really would like to see the maximum complexity to be validated for all heap operations. Especially since they all have different complexities.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128115