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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:47
+    while (__first1 != __last1 && __first2 != __last2) {
+      if (!invoke(__pred, invoke(__proj1, *__first1), invoke(__proj2, *__first2)))
+        break;
----------------
Quuxplusone wrote:
> philnik wrote:
> > Quuxplusone wrote:
> > > `_VSTD::invoke` throughout; and please add a regression test involving `Holder<Incomplete>*`.
> > > https://quuxplusone.github.io/blog/2019/09/26/uglification-doesnt-stop-adl/
> > Using `Holder<Incomplete>*` currently errors in `input_range`, so I won't put a regression test in since fixing `input_range` is out of scope for this PR.
> Yikes, I see this now: https://godbolt.org/z/WPzrhda1f  Is that the problem you mean?  If so, OK, no-regression-test is fine.
Yes.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:88
+
+  test_ranges(b1, b2, Iter(r1 + 3), Iter(r2 + 3));
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117817