Implement std::ranges::swap_ranges()
Details
- Reviewers
• Quuxplusone Mordante ldionne var-const - Group Reviewers
Restricted Project - Commits
- rG9d9053190498: [libc++][ranges] Implement std::ranges::swap_ranges()
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
The code looks pretty solid to me (although I am still requesting to reorganize it). The tests need more fleshing out.
Part of the reason for this PR is to experiment and see whether we want {__algorithm/in_in_result.h, etc} or {__algorithm/results.h}. I would say that AFAICT it provides no interesting evidence for either option, but it's definitely not evidence against my hypothesis that {__algorithm/in_in_result.h, etc} is the right choice. :) The use of in_in_result.h here feels pretty natural and self-documenting.
libcxx/include/__algorithm/swap_ranges.h | ||
---|---|---|
12–19 | The includes needed by std::swap_ranges and std::ranges::swap_ranges practically don't overlap at all, nor are they ever used together, nor is there any circular dependency between them. Therefore (and for other pre-existing prejudices about code organization ;)) I think ranges::swap_ranges should be moved into a new file __algorithm/ranges_swap_ranges.h. | |
16 | Please leave the spacing here consistent with every other header file. (I know, clang-format strikes again; but we have hundreds of header files that use the old spacing, and we shouldn't change them one at a time (or, IMHO, at all).) | |
40–41 | ||
43–44 | ||
48 | ||
libcxx/include/algorithm | ||
669–676 | These changes LGTM, and also IIUC you should remove the comment on line 673 as well. But please make these changes in a separate trivial PR (which I'll "LGTM" as soon as I see it). | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp | ||
76–77 | I suspect that in the process of adding all the below-requested tests, you'll realize that either the templated test2 should go away entirely, or it needs a few more template parameters. Whatever happens, I'd prefer to stay away from circular-looking assertions like this, where we depend on metaprogramming to tell us the answer to some other metaprogramming. Ideally, the tests would be as simple and self-evident as possible, e.g. { std::list<int> r1 = {1, 2, 3}; std::vector<int> r2 = {4, 5, 6}; using It1 = std::list<int>::iterator; using It2 = std::vector<int>::iterator; std::same_as<std::ranges::in_in_result<It1, It2>> auto r = std::ranges::swap_ranges(r1, r2); // Or: auto r = std::ranges::swap_ranges(r1, r2); // ASSERT_SAME_TYPE(decltype(r), std::ranges::in_in_result<It1, It2>); assert(r.in1 == r1.end()); assert(r.in2 == r2.end()); assert((r1 == std::list<int>{4, 5, 6})); assert((r2 == std::vector<int>{1, 2, 3})); } | |
96–102 | (1) I'm not thrilled by the test1, test2, tests1, tests2 naming scheme. I'd prefer a very minor modification: keep test1 and test2, but combine tests[12] into a new constexpr bool test() that does all of the calls to test[12], and then main just does the usual test() / static_assert(test()). Alternatively, maybe the templated test[12] disappear, and then you just rename tests[12] to test_iterator_sentinel and test_range. (2) I don't see any tests for
|
libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp | ||
---|---|---|
96–102 | What non-iterator sentinel types are there? Is somewhere a list of them? I looked at https://en.cppreference.com/w/cpp/header/ranges but didn't see anything of interest there. |
libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp | ||
---|---|---|
96–102 | For our purposes, use sentinel_wrapper<T> from "test_iterators.h". So for example something like this: struct NonCommon { int data_[3] = {}; auto begin() { return forward_iterator<int*>(data_); } auto end() { return sentinel_wrapper<forward_iterator<int*>>(forward_iterator<int*>(data_ + 3)); } }; test_ranges<std::array<int, 3>, NonCommon>(); test_ranges<NonCommon, std::array<int, 3>>(); Just FYI, there are some standard sentinel types: std::default_sentinel and std::unreachable_sentinel, and then there are harder-to-name ones such as std::ranges::sentinel_t<std::ranges::take_while_view<std::string_view, bool(*)(int)>> (actually there's nothing normative to stop that type from also being some kind of iterator, but in practice it never is). |
LGTM % nits!
libcxx/include/__algorithm/ranges_swap_ranges.h | ||
---|---|---|
26 | ||
35–36 | Nit: Consider the following alternatives, either of which feels preferable to me (and the first feels the most "libc++ style": _LIBCPP_HIDE_FROM_ABI constexpr ranges::swap_ranges_result<_I1, _I2> operator()(_I1 __first1, _S1 __last1, _I2 __first2, _S2 __last2) const _LIBCPP_HIDE_FROM_ABI constexpr ranges::swap_ranges_result<_I1, _I2> operator()(_I1 __first1, _S1 __last1, _I2 __first2, _S2 __last2) const | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp | ||
39 | (ditto throughout, just to keep line lengths manageable) | |
53 | Can we get tests here for swap_ranges(j, i) and swap_ranges(j, j+1, i, i+3) as well? | |
157 | Here and line 158-159, please don't line-break between auto and a! Line-break after = would be a good place. Maybe using Expected = std::ranges::swap_ranges_result<...> would be useful. | |
168 | Nit: struct S {}; | |
libcxx/test/std/library/description/conventions/customization.point.object/cpo.compile.pass.cpp | ||
48 ↗ | (On Diff #396976) |
libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp | ||
---|---|---|
85 | IIUC, the special thing about borrowed input ranges is that their iterators don't dangle. So this test should be testing with std::move(b1), not just b1. And that means that it should simply be merged into test_rval_range(), something like std::vector<int> x = {1, 2, 3}; using It = std::vector<int>::iterator; std::same_as<std::ranges::swap_ranges_result<It, It>> auto c = std::ranges::swap_ranges(std::views::all(x), r); assert(c.in1 == x.end()); assert(c.in2 == r.end()); assert((r == std::vector<int>{1, 2, 3})); assert((x == std::vector<int>{4, 5, 6})); where std::views::all(x) is our rvalue, borrowed, range. Going further down this rabbit hole... Notice that both subrange and all_t (i.e. ref_view in this case) are both not only borrowed ranges, but also views. We don't have test coverage for a borrowed non-view. However, such things seem vanishingly rare? I think the trick is to make it immobile: struct BorrowedNonView : std::view_base { BorrowedNonView(BorrowedNonView&&) = delete; int *begin() const; int *end() const; }; I would say that this is not worth it (especially since swap_ranges doesn't do anything surprising w.r.t. dangling). | |
173 | Instead of [[maybe_unused]], let's assert(a.in1 == r.end()). Likewise throughout. | |
179–184 | The only reason this is split out is because vector isn't constexpr yet, right? | |
libcxx/test/std/library/description/conventions/customization.point.object/cpo.compile.pass.cpp | ||
48 ↗ | (On Diff #396976) | This has changed thanks to D116570; please move this diff into libcxx/test/std/library/description/conventions/customization.point.object/niebloid.compile.pass.cpp, where it will be a one-line diff. Actually, now I feel a little guilty for not adding all the iterator-sentinel signatures into niebloid.compile.pass.cpp. I think my original logic was that basically all we were catching with that test was if you did something bizarre with the implementation of the object itself, or if you forgot the const on operator() — but the former doesn't require testing more than one signature, and the latter should be caught immediately by ordinary testing because the object itself is constexpr. |
libcxx/include/__algorithm/ranges_swap_ranges.h | ||
---|---|---|
33–36 | Ditto below. | |
42 | I believe you're missing _VSTD::move on both operands, which means there's some test coverage missing. Please investigate and add regression tests involving cpp20_input_iterator. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp | ||
85 | On the ranges::mismatch review you pointed out that views::all(x) i.e. ref_view is not a borrowed range, and it looks like you're right w.r.t. libc++, but that seems to be a libc++ bug. I think std::ranges::subrange(x) is also acceptable, but it's slightly longer and less idiomatic than views::all. template<class R> struct BorrowedRange { explicit BorrowedRange(const R& r) : r_(&r) {} BorrowedRange(BorrowedRange&&) = delete; // immobile auto begin() const { return std::ranges::begin(*r_); } auto end() const { return std::ranges::end(*r_); } const R *r_; }; static_assert( std::ranges::borrowed_range<BorrowedRange<int[10]>>); static_assert(!std::ranges::view<BorrowedRange<int[10]>>); template<class R> BorrowedRange<R> make_borrowed_range(const R& r) { static_assert(std::ranges::range<const R>); return BorrowedRange<R>(r); } Then line 87 becomes std::ranges::swap_ranges(make_borrowed_range(r1), r2); and it's more self-documenting. | |
123–127 | Consider s/Iter/It/g, s/Sentinel/Sent/g, to shorten these lines. | |
155–163 | Awesome constexprification! But please put each of the two tests in its own { } scope as usual, so Expected2 is defined closer to its point of use. (This requires re-defining r in each scope; I have no problem with that.) |
Please git grep swap_ranges ../libcxx/; I believe there's a TODO in iter_swap related to swap_ranges.
Or if you're planning a followup PR for that, that's also OK.
libcxx/include/__algorithm/ranges_swap_ranges.h | ||
---|---|---|
35 | Here and line 48, we could remove ranges:: from the return type. Should we? | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp | ||
27 | I think the dependency on std::list is gone? Please check whether any other headers can be removed now. | |
142 | Consider linebreaking. | |
211 | s/int, int/int, char/g so we see we're not accidentally swapping the arguments' order or anything crazy like that. Obviously that bug isn't likely, but it's also trivial to test for. |
LGTM modulo a few last test nits.
It might also be nice to
static_assert(!std::is_invocable_v<decltype(std::ranges::swap_ranges), cpp17_output_iterator<int*>, int*>); static_assert(!std::is_invocable_v<decltype(std::ranges::swap_ranges), int*, cpp17_output_iterator<int*>>);
But that's technically non-portable, in that it depends on the ability to take decltype(std::ranges::swap_ranges), which is guaranteed-in-practice-but-not-in-theory because niebloids. So I'm neutral on whether to include those lines somewhere.
libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp | ||
---|---|---|
97 | Nit, throughout: I'd prefer to see std::views::all(rx) instead of std::ranges::subrange(rx). Advantages: shorter, more idiomatic, no CTAD. | |
171 | Above, I'd suggested something involving test_ranges<NonCommon, std::array<int, 3>>(). I no longer particularly care about testing multiple kinds of ranges, because we get some additional coverage in the named tests (e.g. test_borrowed_input_range()). So, let's leave test_range() as testing only std::array alone — but then, please remove its template parameters Range1 and Range2. | |
216 |
Bah, naturally I meant
static_assert(!std::is_invocable_v<decltype(std::ranges::swap_ranges), std::ranges::subrange<cpp17_output_iterator<int*>>, std::ranges::subrange<int*>>); static_assert(!std::is_invocable_v<decltype(std::ranges::swap_ranges), std::ranges::subrange<int*>, std::ranges::subrange<cpp17_output_iterator<int*>>>);