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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/make_heap.h:26
 template <class _Compare, class _RandomAccessIterator>
-_LIBCPP_CONSTEXPR_AFTER_CXX11 void
-__make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp)
-{
-    typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
-    difference_type __n = __last - __first;
-    if (__n > 1)
-    {
-        // start from the first parent, there is no need to consider children
-        for (difference_type __start = (__n - 2) / 2; __start >= 0; --__start)
-        {
-            _VSTD::__sift_down<_Compare>(__first, __comp, __n, __first + __start);
-        }
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
+void __make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp) {
----------------
Note: a few of these existing internal functions were missing the `_LIBCPP_HIDE_FROM_ABI` annotation. I presume adding it won't cause any problems?


================
Comment at: libcxx/include/__algorithm/make_heap.h:41
+void __make_heap_impl(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare& __comp) {
+  using _Comp_ref = typename __comp_ref_type<_Compare>::type;
+  std::__make_heap<_Comp_ref>(std::move(__first), std::move(__last), __comp);
----------------
The main purpose of this function is to avoid leaking the `__comp_ref_type` quirk from this file (similarly for the other heap headers).


================
Comment at: libcxx/include/__algorithm/ranges_pop_heap.h:42
+  _Iter __pop_heap_fn_impl(_Iter __first, _Sent __last, _Comp& __comp, _Proj& __proj) {
+    _LIBCPP_ASSERT(__first != __last, "The heap given to pop_heap must be non-empty");
+    auto __last_iter = ranges::next(__first, __last);
----------------
Note: the Standard specifies that the range has to be non-empty in [[ http://eel.is/c++draft/pop.heap#2 | pop.heap ]]:
```
Preconditions: The range [first, last) is a valid **non-empty** heap
```
However, our existing non-ranges version of `pop_heap` is simply a no-op for an empty range, and the same is true for GCC and MSVC. Please let me know whether you think being strict with this check adds any benefit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128115