[libcxx-commits] [PATCH] D116303: [libc++][ranges] Implement std::ranges::swap_ranges()

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_swap_ranges.h:32-35
+  template <input_iterator _I1, sentinel_for<_I1> _S1,
+            input_iterator _I2, sentinel_for<_I2> _S2>
+  requires indirectly_swappable<_I1, _I2> _LIBCPP_HIDE_FROM_ABI constexpr ranges::swap_ranges_result<_I1, _I2>
+  operator()(_I1 __first1, _S1 __last1, _I2 __first2, _S2 __last2) const {
----------------
Ditto below.


================
Comment at: libcxx/include/__algorithm/ranges_swap_ranges.h:41
+    }
+    return {__first1, __first2};
+  }
----------------
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`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:122-126
+  using Iter = cpp17_input_iterator<int*>;
+  using Sentinel = sentinel_wrapper<Iter>;
+  using Expected = std::ranges::swap_ranges_result<Iter, Iter>;
+  std::same_as<Expected> auto r =
+      std::ranges::swap_ranges(Iter(i), Sentinel(Iter(i + 3)), Iter(j), Sentinel(Iter(j + 3)));
----------------
Consider `s/Iter/It/g`, `s/Sentinel/Sent/g`, to shorten these lines.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:154-162
+  using Expected1 = std::ranges::swap_ranges_result<int*, std::ranges::dangling>;
+  using Expected2 = std::ranges::swap_ranges_result<std::ranges::dangling, int*>;
+  std::array<int, 3> r = {1, 2, 3};
+  std::same_as<Expected1> auto a = std::ranges::swap_ranges(r, std::array{4, 5, 6});
+  assert((r == std::array{4, 5, 6}));
+  assert(a.in1 == r.begin() + 3);
+  std::same_as<Expected2> auto b = std::ranges::swap_ranges(std::array{1, 2, 3}, r);
----------------
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 at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:84
+
+constexpr void test_borrowed_input_range() {
+  int r1[] = {1, 2, 3};
----------------
Quuxplusone wrote:
> 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).
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.
https://en.cppreference.com/w/cpp/ranges/borrowed_range
I'll fix it, and then `views::all(x)` should be acceptable as a borrowed range in these tests.

I think `std::ranges::subrange(x)` is also acceptable, but it's slightly longer and less idiomatic than `views::all`.
Actually perhaps we should make a new dedicated test type, in `support/test_range.h`:
```
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.
However, the existing `support/test_range.h` [sic] looks a little messy, so maybe I'll tackle that at some point this week myself, unless you really //want// to tackle it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116303