[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 comment.

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.



================
Comment at: libcxx/include/__algorithm/ranges_swap_ranges.h: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 {
----------------
Here and line 48, we //could// remove `ranges::` from the return type. Should we?
Let's be consistent with whatever you're doing in `ranges::mismatch` and any other open (or closed) PRs.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:27
+#include <cassert>
+#include <list>
+#include <memory>
----------------
I think the dependency on `std::list` is gone? Please check whether any other headers can be removed now.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:142
+  int j[3] = {4, 5, 6};
+  std::same_as<Expected> auto r = std::ranges::swap_ranges(Iter1(i), sentinel_wrapper(Iter1(i + 3)), Iter2(j), sentinel_wrapper(Iter2(j + 3)));
+  assert(base(r.in1) == i + 3);
----------------
Consider linebreaking.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:211
+
+static_assert(std::same_as<std::ranges::swap_ranges_result<int, int>, std::ranges::in_in_result<int, int>>);
+
----------------
`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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116303