- ranges::make_heap;
- ranges::push_heap;
- ranges::pop_heap;
- ranges::sort_heap.
Details
- Reviewers
jdoerfert huixie90 - Group Reviewers
Restricted Project - Commits
- rGc945bd0da652: [libc++][ranges] Implement modifying heap algorithms:
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/__algorithm/make_heap.h | ||
---|---|---|
26–33 | 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? | |
41 | The main purpose of this function is to avoid leaking the __comp_ref_type quirk from this file (similarly for the other heap headers). | |
libcxx/include/__algorithm/ranges_pop_heap.h | ||
43 | Note: the Standard specifies that the range has to be non-empty in 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. |
libcxx/include/__algorithm/make_heap.h | ||
---|---|---|
26–33 | 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. | |
libcxx/include/__algorithm/ranges_make_heap.h | ||
47 | Probably applies elsewhere too. | |
libcxx/include/__algorithm/ranges_pop_heap.h | ||
43 | 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. |
libcxx/include/__algorithm/ranges_pop_heap.h | ||
---|---|---|
43 | Just to clarify, do you mean also adding it to the classic algorithm, or just to the ranges version? In the former case, would that require a release note? |
libcxx/include/__algorithm/make_heap.h | ||
---|---|---|
33 | I think __sift_down does not use iter_move or iter_swap. This is not correct for ranges algorithms | |
libcxx/include/__algorithm/pop_heap.h | ||
33 | I believe this needs to call ranges::iter_move for the ranges version | |
40 | same as above | |
43 | I think this does not do the right thing for ranges overload | |
libcxx/include/__algorithm/push_heap.h | ||
35–37 | should use iter_move for ranges overload | |
libcxx/include/__algorithm/sort_heap.h | ||
31 | This won't call the ranges::iter_move for the ranges overload |
Address feedback.
libcxx/include/__algorithm/pop_heap.h | ||
---|---|---|
33 | Thanks _a lot_ for finding this! I'd prefer to address this for all recently-implemented algorithms in a follow-up patch -- would that work for you? | |
libcxx/include/__algorithm/ranges_pop_heap.h | ||
43 | From offline discussion: we should add an assertion for the classic version as well. A release note is not necessary because it's undefined behavior. |
libcxx/include/__algorithm/pop_heap.h | ||
---|---|---|
33 | Yes. sounds good to me. |
libcxx/include/__algorithm/make_heap.h | ||
---|---|---|
40 | I would suggest:
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. | |
libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/pop.heap/assert.pop_heap.pass.cpp | ||
10 | 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. | |
libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/pop.heap/assert.ranges_pop_heap.pass.cpp | ||
15–24 | I am not sure that the full synopsis is useful here. Also applies to other assert.FOO tests. |
Address feedback.
libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/pop.heap/assert.pop_heap.pass.cpp | ||
---|---|---|
10 | Thanks! |
LGTM. I think you changed some signatures of some __ function to take an "Tag" template argument in the partial_sort patch and this change might need to rebase that change?
libcxx/include/algorithm | ||
---|---|---|
276 | unresolved merge conflicts? |
In general looks good, but there are some small issues.
libcxx/include/__algorithm/make_heap.h | ||
---|---|---|
39–48 | Maybe drop the inline while you're at it. | |
libcxx/include/__algorithm/push_heap.h | ||
39 | Can you put the break; on its own line? | |
53 | You need to calculate __last - __first before the function call since they are moved from in this call. | |
libcxx/include/__algorithm/ranges_pop_heap.h | ||
49 | Here __first might be used in a moved from state. | |
libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp | ||
44 | I think it would be good to test with all iterator categories to see only random access and contiguous are valid. | |
218 | I really would like to see the maximum complexity to be validated for all heap operations. Especially since they all have different complexities. |
Address feedback, fix the CI.
libcxx/include/__algorithm/make_heap.h | ||
---|---|---|
39–48 | This seems like more than a purely mechanical change; I'd rather keep as is. | |
libcxx/include/__algorithm/push_heap.h | ||
53 | Thanks! | |
libcxx/include/algorithm | ||
276 | Sorry about that! | |
libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp | ||
44 | This would apply to every test range algorithm test and make it more verbose without increasing coverage. The types from almost_satisfies_types.h are deliberately written to fall just short of meeting constraints of the type while satisfying all the prerequisite constraints; e.g. both RandomAccessIteratorNotDerivedFrom and RandomAccessIteratorBadIndex satisfy bidirectional_iterator. Thus, if RandomAccessIteratorNotDerivedFrom doesn't satisfy the constraints, it means that bidirectional_iterator and all the iterator concepts it refines don't satisfy, either. | |
218 | I think this is valuable but I'd rather do a follow-up after we finished the initial implementation of range algorithms dedicated solely to testing complexity where possible. |
A few comments other than that LGTM! But I like to see the CI green before approving.
libcxx/include/__algorithm/push_heap.h | ||
---|---|---|
53 | I would prefer not to use auto. | |
libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp | ||
44 | Fair point. | |
218 | Should we add a note on the Status page as a reminder? |
libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp | ||
---|---|---|
218 | I'll get to this pretty soon -- I don't think there should be need for a TODO. |
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?