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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:62
 Write,partition_copy,Konstantin Varlamov,`D130070 <https://llvm.org/D130070>`_,✅
-Write,partial_sort_copy,Konstantin Varlamov,n/a,In progress
+Write,partial_sort_copy,Konstantin Varlamov,`TODO <https://llvm.org/TODO>`_,✅
 Merge,merge,Hui Xie,`D128611 <https://llvm.org/D128611>`_,✅
----------------
huixie90 wrote:
> can fill in the patch number
Thanks!


================
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,
----------------
huixie90 wrote:
> 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?
In either case, I've added it to my backlog to prepare a `robust` test that we support single-pass input iterators in all the relevant algorithms.


================
Comment at: libcxx/include/__algorithm/partial_sort_copy.h:53
             }
-        std::__sort_heap<_AlgPolicy, _Compare>(__result_first, __r, __comp);
+        std::__sort_heap<_AlgPolicy, _Compare>(__result_first, __r, __comp, __proj2);
     }
----------------
ldionne wrote:
> As we discussed just now, IMO this is better since it shields `__sort_heap` & friends from having to know about projections. I don't have a strong preference though, I'll defer to your judgement/preference.
Unfortunately, for this to work, `__make_projected_comp` would have to support C++03. I took a stab at it: https://reviews.llvm.org/D130611, and it's a bit ugly. Let's discuss in the linked patch -- I can submit it as a follow up.


================
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));
 }
----------------
huixie90 wrote:
> 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?
>From the offline discussion, this is probably unnecessary because `__first` should already point to `__last`. I'm a little reluctant to remove this from this patch, though -- it seems like it should be a no-op in that case, but if the assumption is somehow wrong, the function would be broken. I'll do a follow-up instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130532