[libcxx-commits] [PATCH] D130532: [libc++][ranges] Implement `std::ranges::partial_sort_copy`.

Tue Jul 26 13:25:49 PDT 2022

huixie90 added inline comments.

================
Comment at: libcxx/include/__algorithm/partial_sort_copy.h:36
+_LIBCPP_CONSTEXPR_AFTER_CXX17 pair<_InputIterator, _RandomAccessIterator>
+__partial_sort_copy(_InputIterator __first, _Sentinel1 __last,
+                    _RandomAccessIterator __result_first, _Sentinel2 __result_last,
----------------
ldionne wrote:
> This is a pre-existing issue, but I think our implementation doesn't work for single-pass input iterators, since we go over the input range twice. Since this is a pre-existing issue, let's fix it after the dust has settled with the release, but let's please make sure to remember. We should start filing bugs for issues like this that we encounter, to make sure they are not forgotten.
@ldionne, Could you please clarify a bit more? the input range `[__first, __last)` seems to be iterated only once unless I missed anything?

================
Comment at: libcxx/include/__algorithm/partial_sort_copy.h:57
+    return pair<_InputIterator, _RandomAccessIterator>(
+        _IterOps<_AlgPolicy>::next(std::move(__first), std::move(__last)), std::move(__r));
 }
----------------
ldionne wrote:
> We are recomputing `next(__first, __last)` here, however this wouldn't work for a single-pass input iterator. Fortunately, we just computed that in the loop above, so it should be possible to extract it from there. Definitely not for this patch, as this will fail when we have a test for single-pass input iterators anyway.
@ldionne, I don't think we are recomputing it or multi-passing the range. `__first` at this stage is already pointing to very end if not the `__last`. We are never going to iterating from the beginning but only iterating from the point that we haven't iterated yet. or do I miss anything?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130532