Implement std::ranges::swap_ranges()
Details
Details
- Reviewers
• Quuxplusone Mordante ldionne var-const - Group Reviewers
Restricted Project - Commits
- rG9d9053190498: [libc++][ranges] Implement std::ranges::swap_ranges()
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
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.) | |
Comment Actions
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 | ||
|---|---|---|
| 36 | 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 | ||
| 28 | I think the dependency on std::list is gone? Please check whether any other headers can be removed now. | |
| 143 | Consider linebreaking. | |
| 212 | 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. | |
Comment Actions
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 | ||
Comment Actions
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*>>>);