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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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) {
----------------
var-const wrote:
> 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?
It must have been because they didn't want to `always_inline` those functions back in the days. Adding it is the correct thing to do since we don't use `always_inline` for ABI anymore.


================
Comment at: libcxx/include/__algorithm/ranges_make_heap.h:46
+
+    auto&& __projected_comp = __make_projected_comp(__comp, __proj);
+    std::__make_heap_impl(std::move(__first), __last_iter, __projected_comp);
----------------
Probably applies elsewhere too.


================
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);
----------------
var-const wrote:
> 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.
I would add a `_LIBCPP_ASSERT` to be strict about this. Users are likely to use the last element of the range after calling `pop_heap` (because that's the element they popped), and that would be UB if our `pop_heap` returns silently when the range is empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128115