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

Tue Dec 28 16:16:48 PST 2021

Quuxplusone requested changes to this revision.
Quuxplusone added a subscriber: jloser.
Quuxplusone added a comment.
This revision now requires changes to proceed.

The code looks pretty solid to me (although I am still requesting to reorganize it). The tests need more fleshing out.

Part of the reason for this PR is to experiment and see whether we want {`__algorithm/in_in_result.h`, etc} or {`__algorithm/results.h`}.  I would say that AFAICT it provides no interesting evidence //for// either option, but it's definitely not evidence //against// my hypothesis that {`__algorithm/in_in_result.h`, etc} is the right choice. :) The use of `in_in_result.h` here feels pretty natural and self-documenting.

================
Comment at: libcxx/include/__algorithm/swap_ranges.h:12-19
+#include <__algorithm/in_in_result.h>
 #include <__config>
+#include <__iterator/concepts.h>
+#include <__iterator/iter_swap.h>
+#include <__ranges/concepts.h>
+#include <__ranges/dangling.h>
 #include <__utility/swap.h>
----------------
The includes needed by `std::swap_ranges` and `std::ranges::swap_ranges` practically don't overlap at all, nor are they ever used together, nor is there any circular dependency between them. Therefore (and for other pre-existing prejudices about code organization ;)) I think `ranges::swap_ranges` should be moved into a new file `__algorithm/ranges_swap_ranges.h`.

================
Comment at: libcxx/include/__algorithm/swap_ranges.h:22
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-#pragma GCC system_header
+#  pragma GCC system_header
 #endif
----------------
Please leave the spacing here consistent with every other header file. (I know, clang-format strikes again; but we have //hundreds// of header files that use the old spacing, and we shouldn't change them one at a time (or, IMHO, at all).)

================
Comment at: libcxx/include/__algorithm/swap_ranges.h:46-47
+    requires indirectly_swappable<_I1, _I2>
+  constexpr ranges::swap_ranges_result<_I1, _I2> operator()(_I1 __first1, _S1 __last1,
+                                                            _I2 __first2, _S2 __last2) const
+  {
----------------

================
Comment at: libcxx/include/__algorithm/swap_ranges.h:49-50
+  {
+    while (__first1 != __last1 &&
+           __first2 != __last2)
+    {
----------------

================
Comment at: libcxx/include/__algorithm/swap_ranges.h:54
+      ++__first1;
+       ++__first2;
+    }
----------------

================
Comment at: libcxx/include/algorithm:662-669
+#include <__bits> // __libcpp_clz
 #include <__config>
 #include <__debug>
-#include <__bits> // __libcpp_clz
 #include <cstddef>
 #include <cstring>
 #include <functional>
 #include <initializer_list>
----------------
These changes LGTM, and also IIUC you should remove the comment on line 673 as well. But please make these changes in a separate trivial PR (which I'll "LGTM" as soon as I see it).

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:75-76
+
+  auto r = std::ranges::swap_ranges(r1, r2);
+  ASSERT_SAME_TYPE(decltype(r), std::ranges::in_in_result<std::ranges::iterator_t<Range>, std::ranges::iterator_t<Range>>);
+  assert(r.in1 == std::ranges::end(r1));
----------------
I suspect that in the process of adding all the below-requested tests, you'll realize that //either// the templated `test2` should go away entirely, //or// it needs a few more template parameters. Whatever happens, I'd prefer to stay away from circular-looking assertions like this, where we depend on metaprogramming to tell us the answer to some other metaprogramming. Ideally, the tests would be as simple and self-evident as possible, e.g.
```
{
std::list<int> r1 = {1, 2, 3};
std::vector<int> r2 = {4, 5, 6};
using It1 = std::list<int>::iterator;
using It2 = std::vector<int>::iterator;
std::same_as<std::ranges::in_in_result<It1, It2>> auto r =
    std::ranges::swap_ranges(r1, r2);
// Or: auto r = std::ranges::swap_ranges(r1, r2);
//     ASSERT_SAME_TYPE(decltype(r), std::ranges::in_in_result<It1, It2>);
assert(r.in1 == r1.end());
assert(r.in2 == r2.end());
assert((r1 == std::list<int>{4, 5, 6}));
assert((r2 == std::vector<int>{1, 2, 3}));
}
```

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp:95-101
+int main(int, char**) {
+  tests1();
+  tests2();
+  static_assert(tests1());
+  static_assert(tests2());
+  return 0;
+}
----------------
(1) I'm not thrilled by the `test1, test2, tests1, tests2` naming scheme. I'd prefer a very minor modification: keep `test1` and `test2`, but combine `tests[12]` into a new `constexpr bool test()` that does //all// of the calls to `test[12]`, and then `main` just does the usual `test() / static_assert(test())`. Alternatively, maybe the templated `test[12]` disappear, and then you just rename `tests[12]` to `test_iterator_sentinel` and `test_range`.

(2) I don't see any tests for
- non-contiguous range types
- non-iterator sentinel types
- two inputs that aren't of the same length
- non-forward iterators (e.g. `cpp20_input_iterator` and `cpp17_input_iterator`), and ranges thereof
- borrowed range types e.g. `subrange` (right?)
- non-borrowed range types as rvalues, which IIUC should return `swap_ranges_result<dangling, Iter2>` or `swap_ranges_result<Iter1, dangling>` so let's test that
- SFINAE-friendliness (I believe you can-and-should just add a line to @jloser's `cpo.compile.pass.cpp`, or optionally start a new sibling file specifically for the ranges algorithms)
- the non-constrainedness of `swap_ranges_result`, e.g. `swap_ranges_result<int, int> p;` is a perfectly fine declaration (Admittedly this is basically implied by the fact that `swap_ranges_result<dangling, dangling>` is OK — `dangling` is not an iterator either — but IMO let's test it anyway just for the record. It's the least important of these items by far, though)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116303