[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/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);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128115