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

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

Quuxplusone added a comment.

Just test nits at this point (probably all related to inlining `test_ranges` :))



================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:38-45
+  _LIBCPP_HIDE_FROM_ABI constexpr
+  ranges::mismatch_result<_I1, _I2> operator()(_I1 __first1,
+                                               _S1 __last1,
+                                               _I2 __first2,
+                                               _S2 __last2,
+                                               _Pred __pred = {},
+                                               _Proj1 __proj1 = {},
----------------
Quuxplusone wrote:
> 
Just a note to say this implementation LGTM now, and awesome, I see that testing with `cpp20_input_iterator` caught a bug :)


================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:47
+    while (__first1 != __last1 && __first2 != __last2) {
+      if (!invoke(__pred, invoke(__proj1, *__first1), invoke(__proj2, *__first2)))
+        break;
----------------
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.


================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:36
+struct __fn {
+  template <class _I1, class _S1, class _I2, class _S2, class _Pred, class _Proj1, class _Proj2>
+  static _LIBCPP_HIDE_FROM_ABI constexpr
----------------
Here and line 39, consider linebreaking after `_S2,`. These lines are //just barely// too long for Phabricator, so this is a pretty minor nit; but it's also super easy to fix.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:23-27
+  if constexpr (!std::is_same_v<Iter1, cpp20_input_iterator<int*>> &&
+                !std::is_same_v<Iter2, cpp20_input_iterator<int*>>) {
+    assert(ret.in1 == expected1);
+    assert(ret.in2 == expected2);
+  }
----------------
And change the signature on line 19 to take `int *expected1, int *expected2`.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117817