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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a subscriber: ldionne.
Quuxplusone added a comment.

LGTM % nits!



================
Comment at: libcxx/include/__algorithm/ranges_swap_ranges.h:25
+
+namespace ranges {
+template <class _I1, class _I2>
----------------



================
Comment at: libcxx/include/__algorithm/ranges_swap_ranges.h:34-35
+    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
+  {
----------------
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
```


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:38
+  int j[1] = {4};
+  std::same_as<std::ranges::swap_ranges_result<int*, int*>> auto r = std::ranges::swap_ranges(i, i + 3, j, j + 1);
+  assert(r.in1 == i + 1);
----------------
(ditto throughout, just to keep line lengths manageable)


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:52
+  assert(j[0] == 4);
+}
+
----------------
Can we get tests here for `swap_ranges(j, i)` and `swap_ranges(j, j+1, i, i+3)` as well?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:156
+  [[maybe_unused]] std::same_as<std::ranges::swap_ranges_result<std::vector<int>::iterator, std::ranges::dangling>> auto
+      a = std::ranges::swap_ranges(r, std::vector{4, 5, 6});
+  assert((r == std::vector{4, 5, 6}));
----------------
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.
Also, please `std::vector<int>` rather than just `std::vector`. (I'm unusually anti-CTAD by nature, but) let's not introduce additional complexity if we don't have to.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:167
+
+  struct S{};
+  ASSERT_SAME_TYPE(std::ranges::swap_ranges_result<S, S>, std::ranges::in_in_result<S, S>);
----------------
Nit: `struct S {};`
Or just use `int` on line 168 and then you don't need `S`.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116303