Details
- Reviewers
jdoerfert huixie90 ldionne - Group Reviewers
Restricted Project - Commits
- rGdb7d7959787e: [libc++][ranges] Implement `std::ranges::partial_sort_copy`.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
LGTM % comments and CI
libcxx/include/__algorithm/partial_sort_copy.h | ||
---|---|---|
34 | 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. | |
51 | 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. | |
55 | 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. | |
libcxx/test/std/algorithms/ranges_robust_against_differing_projections.pass.cpp | ||
2 | This is part of another commit. |
libcxx/include/__algorithm/partial_sort_copy.h | ||
---|---|---|
34 | @ldionne, Could you please clarify a bit more? the input range [__first, __last) seems to be iterated only once unless I missed anything? | |
55 | @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? |
Address feedback and rebase.
libcxx/docs/Status/RangesAlgorithms.csv | ||
---|---|---|
62 | Thanks! | |
libcxx/include/__algorithm/partial_sort_copy.h | ||
34 | 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. | |
51 | 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. | |
55 | 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. |
libcxx/include/__algorithm/make_projected.h | ||
---|---|---|
81–84 ↗ | (On Diff #448217) | why is the enable_if logic written twice? template <class A, enable_if<C>* = nullptr> |
Address feedback and rebase.
libcxx/include/__algorithm/make_projected.h | ||
---|---|---|
81–84 ↗ | (On Diff #448217) | Thanks for spotting! |
can fill in the patch number