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

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.

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.



================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:96
+    int r2[] = {4, 5, 6};
+    std::ranges::swap_ranges(r1, std::ranges::subrange(r2));
+    assert(r1[0] == 4);
----------------
Nit, throughout: I'd prefer to see `std::views::all(rx)` instead of `std::ranges::subrange(rx)`. Advantages: shorter, more idiomatic, no CTAD.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:170
+constexpr bool test() {
+  test_range<std::array<int, 3>, std::array<int, 3>>();
+
----------------
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`.
Also consider using C arrays instead of `std::array`, just to keep things simple.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:215
+  static_assert(test());
+  return 0;
+}
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116303