[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/pop_heap.h:33
+  if (__len > 1) {
+    value_type __top = std::move(*__first);  // create a hole at __first
+    _RandomAccessIterator __hole = std::__floyd_sift_down<_Compare>(__first, __comp, __len);
----------------
huixie90 wrote:
> I believe this needs to call `ranges::iter_move` for the ranges version
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?


================
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:
> ldionne wrote:
> > 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.
> 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?
>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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128115