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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


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


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:172
+  std::vector<int> r = {1, 2, 3};
+  [[maybe_unused]] std::same_as<Expected1> auto a = std::ranges::swap_ranges(r, std::vector{4, 5, 6});
+  assert((r == std::vector{4, 5, 6}));
----------------
Instead of `[[maybe_unused]]`, let's `assert(a.in1 == r.end())`. Likewise throughout.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:178-183
+void test() {
+  test_range<std::vector<int>, std::list<int>>();
+  test_rval_range();
+
+  static_assert(std::same_as<std::ranges::swap_ranges_result<int, int>, std::ranges::in_in_result<int, int>>);
+}
----------------
The only reason this is split out is because `vector` isn't constexpr yet, right?
Could you see whether it's easy to use the constexpr deque from `libcxx/test/support/test_constexpr_container.h` instead?


================
Comment at: libcxx/test/std/library/description/conventions/customization.point.object/cpo.compile.pass.cpp:48
+static_assert(test(std::ranges::swap_ranges, a, a));
+static_assert(test(std::ranges::swap_ranges, a, a + 1, a, a + 1));
+
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > Utter nit, but let's make this `a + 10` in both cases, since this signifies an "end" iterator. :)
> This is relevant to @ldionne's interests re that LWG (write-a-paper-not-an-)issue. @ldionne, should we start testing our niebloids in here? start a new test file? none of the above?
> This might be a blocking/controversial point.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116303