[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:40
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
+void __make_heap_impl(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare& __comp) {
+  using _Comp_ref = typename __comp_ref_type<_Compare>::type;
----------------
I would suggest:
1. Swapping the names `__make_heap_impl` and `__make_heap`.
2. Inlining the new `__make_heap_impl` into the new `__make_heap`
3. Removing `__make_heap_impl`, which isn't used anywhere anymore.

This gives:

```
template <class _RandomAccessIterator, class _Compare>
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
void __make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare& __comp) {
  using _Comp_ref = typename __comp_ref_type<_Compare>::type;
  _Comp_ref __comp_ref = __comp;

  using difference_type = typename iterator_traits<_RandomAccessIterator>::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) {
        std::__sift_down<_Compare>(__first, __comp_ref, __n, __first + __start);
    }
  }
}
```

I think the same comment applies to all the other heap algorithms.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/pop.heap/assert.pop_heap.pass.cpp:10
+// REQUIRES: has-unix-headers
+// UNSUPPORTED: !libcpp-has-debug-mode
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
----------------
This is not needed -- we support basic assertions even when the "debug" mode is disabled.

It's kind of messy right now (I'm in the process of cleaning that up), but basically Debug Mode = heavyweight checks that change ABI and/or complexity, and Assertions = lightweight checks that don't impact ABI/complexity.

Debug Mode implies Assertions, but not the other way around. Here you only need to turn Assertions on with `_LIBCPP_ENABLE_ASSERTIONS=1` to test this.

This comment applies to the other `assert.FOO` tests.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/pop.heap/assert.ranges_pop_heap.pass.cpp:15-24
+// template<random_access_iterator I, sentinel_for<I> S, class Comp = ranges::less,
+//         class Proj = identity>
+//   requires sortable<I, Comp, Proj>
+//   constexpr I
+//     ranges::pop_heap(I first, S last, Comp comp = {}, Proj proj = {});                    // since C++20
+//
+// template<random_access_range R, class Comp = ranges::less, class Proj = identity>
----------------
I am not sure that the full synopsis is useful here. Also applies to other `assert.FOO` tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128115