[libcxx-commits] [PATCH] D117817: [libc++][ranges] Implement ranges::mismatch

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:88
+
+  test_ranges(b1, b2, Iter(r1 + 3), Iter(r2 + 3));
+}
----------------
philnik wrote:
> Quuxplusone wrote:
> > You can replace this call to `test_ranges` with simply
> > ```
> >   using Expected = std::ranges::mismatch_result<Iter, Iter>;
> >   std::same_as<Expected> auto ret = std::ranges::mismatch(b1, b2);
> >   assert(base(ret.in1) == r1 + 3);
> >   assert(base(ret.in2) == r2 + 3);
> > ```
> > (leaving off the `base()` if you want, since here `Iter` is hardcoded to `int*` already).
> > However, this doesn't seem to match the name of the test case... it's supposed to be testing a borrowed range, right? The thing that's special about borrowed ranges is that they work as rvalues. So what you should be testing here is rvalues. I continue to recommend `views::all(r1)` because it's easy to type.
> > ```
> > constexpr void test_borrowed_range() {
> >   int r1[] = {1, 2, 3, 4, 5, 6};
> >   int r2[] = {1, 2, 3, 5, 4};
> >   {
> >     // non-borrowed lvalue vs. borrowed rvalue
> >     std::same_as<std::ranges::mismatch_result<int*, int*>> auto ret =
> >       std::ranges::mismatch(r1, std::views::all(r2));
> >     assert(ret.in1 == r1 + 3);
> >     assert(ret.in2 == r2 + 3);
> >   }
> >   {
> >     // borrowed rvalue vs. non-borrowed lvalue
> >     std::same_as<std::ranges::mismatch_result<int*, int*>> auto ret =
> >       std::ranges::mismatch(std::views::all(r1), r2);
> >     assert(ret.in1 == r1 + 3);
> >     assert(ret.in2 == r2 + 3);
> >   }
> >   {
> >     // borrowed rvalue vs. borrowed rvalue
> >     std::same_as<std::ranges::mismatch_result<int*, int*>> auto ret =
> >       std::ranges::mismatch(std::views::all(r1), std::views::all(r2));
> >     assert(ret.in1 == r1 + 3);
> >     assert(ret.in2 == r2 + 3);
> >   }
> > }
> > ```
> > (the `// comments` are optional)
> > 
> > I'm hoping that seeing this example changes your mind about the utility of inlining `test_ranges` everywhere else. I haven't dug into those call-sites yet myself, and am hoping you will.
> `std::views::all` seems to not be a borrowed_range. At least it's not a borrowed range in libc++ and I couldn't find anything in the standard.
D118164 fixed the borrowed-ness of `views::all(r)`, so you can do this now too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117817