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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % nits.



================
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:
> > Quuxplusone wrote:
> > > philnik wrote:
> > > > 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.
> > > This is now D118213, so hopefully by end-of-week you can add that regression test after all. :)
> > > ```
> > > Holder<Incomplete> *a[3] = {};
> > > Holder<Incomplete> *b[4] = {};
> > > auto [ai, bi] = std::ranges::mismatch(a, b);
> > > assert(ai == a+3);
> > > assert(bi == b+3);
> > > ```
> > D118213 is landed! So you should be able to add the above test now.
> Now it fails in `iterator_traits.h:150`. I don't really understand why and I also don't feel like fixing all the concepts before landing this.
Discussed offline, and discovered that Ranges algorithms are fundamentally not ADL-proof, so we actively //cannot// add the test I was asking for.

https://godbolt.org/z/P9fhhYWvW

I actually now have a suggested fix to ADL-firewall `std::projected`, but it will require a paper.


================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:30
+
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
This will need merge-conflict resolution. I suggest just this for now:
```
#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
```
although if we wait long enough, D118736 might change it again.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:65
+
+  test_iterators(a.begin(), a.begin() + 3, b.begin(), b.begin() + 2, a.begin() + 2, b.begin() + 2);
+  using Expected = std::ranges::mismatch_result<int*, int*>;
----------------
`a.begin() + 3` is a funny way to spell `a.end()`.
How about changing `a` and `b` to C arrays, and then you can just say `test_iterators(a, a + 3, b, b + 2, a + 2, b + 2)`?

Please also test when the first range is shorter, i.e. add `test_iterators(b, b + 2, a, a + 3, b + 2, a + 2)`.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:167
+  test();
+  static_assert(test());
+  return 0;
----------------
I claim that we traditionally put a blank line before the `return 0;`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117817