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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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;
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116303