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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Jun 20 11:17:14 PDT 2022

ldionne added inline comments.

Comment at: libcxx/include/__algorithm/make_heap.h:26
 template <class _Compare, class _RandomAccessIterator>
-__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);
-        }
+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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list