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

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Thanks for working on this!



================
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 = {},
----------------



================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:47
+    while (__first1 != __last1 && __first2 != __last2) {
+      if (!invoke(__pred, invoke(__proj1, *__first1), invoke(__proj2, *__first2)))
+        break;
----------------
`_VSTD::invoke` throughout; and please add a regression test involving `Holder<Incomplete>*`.
https://quuxplusone.github.io/blog/2019/09/26/uglification-doesnt-stop-adl/


================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:58-59
+    requires indirectly_comparable<iterator_t<_R1>, iterator_t<_R2>, _Pred, _Proj1, _Proj2>
+  _LIBCPP_HIDE_FROM_ABI constexpr ranges::mismatch_result<borrowed_iterator_t<_R1>, borrowed_iterator_t<_R2>>
+      operator()(_R1&& __r1, _R2&& __r2, _Pred&& __pred = {}, _Proj1&& __proj1 = {}, _Proj2&& __proj2 = {}) const {
+    return operator()(ranges::begin(__r1),
----------------



================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:64-66
+                      std::forward<_Pred>(__pred),
+                      std::forward<_Proj1>(__proj1),
+                      std::forward<_Proj2>(__proj2));
----------------
Here, either `std::move(__foo)` or, better, rejigger this so that we can use references (or our `__comp_ref` trick) with all of these callables. The Standard permits us to copy these callables as much as we like (AFAIK); but as a quality-of-implementation thing, we want to make zero copies and ideally (IMHO) zero moves as well.

I think it's easy to make this work; you just have to pull out the algorithm itself into a helper
```
template<class _I1, class _S1, class _I2, class _S2,
         class _Pred, class _Proj1, class _Proj2>
static _LIBCPP_HIDE_FROM_ABI constexpr
mismatch_result<_I1, _I2>
__go(_I1 __first1, _S1 __last1, _I2 __first2, _S2 __last2,
     _Pred& __pred, _Proj1& __proj1, _Proj2& __proj2) const {
  while (~~~
  return {__first1, __first2};
}
```
and then call `__go` from both overloads of `operator()`, e.g.:
```
~~~
mismatch_result<_I1, _I2>
operator()(_I1 __first1, _S1 __last1, _I2 __first2, _S2 __last2,
           _Pred __pred = {}, _Proj1 __proj1 = {}, _Proj2 __proj2 = {}) const {
  return __go(_VSTD::move(__first1), __last1, _VSTD::move(__first2), __last2,
               __pred, __proj1, __proj2);
}
```


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Please uncomment the relevant line in `std/library/description/conventions/customization.point.object/niebloid.compile.pass.cpp`.
Consider adding explicit test coverage here for C arrays, e.g.
```
int a[] = {1, 2, 3, 4};
int b[] = {1, 2, 4, 8, 16};
auto [ai, bi] = std::ranges::mismatch(a, b);
assert(ai == a + 2);
assert(bi == b + 2);
auto [aj, bj] = std::ranges::mismatch(a, a+4, b, b+5);
assert(aj == a + 2);
assert(bj == b + 2);
```
This is on my mind tonight because of D117940; but I don't think there's anything //specific// to `mismatch` that would bite us here, and `niebloid.compile.pass.cpp` already (by a happy accident) tests the C-array case.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:16-30
+template <class Iter1, class Iter2>
+constexpr void test_iterators(Iter1 begin1, Iter1 end1, Iter2 begin2, Iter2 end2, Iter1 expected1, Iter2 expected2) {
+  using Expected = std::ranges::mismatch_result<Iter1, Iter2>;
+  std::same_as<Expected> auto ret = std::ranges::mismatch(begin1, end1, begin2, end2);
+  assert(ret.in1 == expected1);
+  assert(ret.in2 == expected2);
+}
----------------
I think this layer of indirection is more confusing than helpful. Strongly recommend inlining these assertions directly into `test_iters`, `test_different_lengths`, and `test_borrowed_range` — I think that will probably even shorten the code! 


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:75
+constexpr bool test() {
+  test_iters<forward_iterator<int*>, forward_iterator<int*>>();
+  test_iters<forward_iterator<int*>, bidirectional_iterator<int*>>();
----------------
`mismatch`'s constraint says `input_iterator`, so you need to test with both `cpp17_input_iterator` and (especially) `cpp20_input_iterator`, as well as all these.
Please also throw in `contiguous_iterator<int*>`, since there's no good reason to leave it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117817