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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM % comments and CI



================
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,
----------------
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.


================
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);
     }
----------------
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.


================
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));
 }
----------------
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.


================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_differing_projections.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
This is part of another commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130532