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

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

Quuxplusone added inline comments.


================
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;
+}
----------------
philnik wrote:
> Quuxplusone wrote:
> > (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)
> What non-iterator sentinel types are there? Is somewhere a list of them? I looked at https://en.cppreference.com/w/cpp/header/ranges but didn't see anything of interest there.
For our purposes, use `sentinel_wrapper<T>` from `"test_iterators.h"`. So for example something like this:
```
struct NonCommon {
  int data_[3] = {};
  auto begin() { return forward_iterator<int*>(data_); }
  auto end() { return sentinel_wrapper<forward_iterator<int*>>(forward_iterator<int*>(data_ + 3)); }
};
test_ranges<std::array<int, 3>, NonCommon>();
test_ranges<NonCommon, std::array<int, 3>>();
```

Just FYI, there are some standard sentinel types: `std::default_sentinel` and `std::unreachable_sentinel`, and then there are harder-to-name ones such as `std::ranges::sentinel_t<std::ranges::take_while_view<std::string_view, bool(*)(int)>>` (actually there's nothing normative to //stop// that type from also being some kind of iterator, but in practice it never is).


================
Comment at: libcxx/test/std/library/description/conventions/customization.point.object/cpo.compile.pass.cpp:46
 
+// [alg.modifiying.operations]
+static_assert(test(std::ranges::swap_ranges, a, a));
----------------



================
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));
+
----------------
Utter nit, but let's make this `a + 10` in both cases, since this signifies an "end" iterator. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116303