Implement ranges::mismatch
Details
- Reviewers
• Quuxplusone ldionne var-const Mordante - Group Reviewers
Restricted Project - Commits
- rGc2cd15a66531: [libc++][ranges] Implement ranges::mismatch
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Drive-by comment: can you also update Status/RangesPaper.csv and Status/RangesAlgorithms.csv?
libcxx/include/module.modulemap | ||
---|---|---|
287 | Drive-by comment: s/rnages/ranges/ |
Thanks for working on this!
libcxx/include/__algorithm/ranges_mismatch.h | ||
---|---|---|
39–46 | ||
48 | _VSTD::invoke throughout; and please add a regression test involving Holder<Incomplete>*. | |
59–60 | ||
65–67 | 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); } | |
libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp | ||
2 | Please uncomment the relevant line in std/library/description/conventions/customization.point.object/niebloid.compile.pass.cpp. 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. | |
17–31 | 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! | |
76 | 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. |
- Rebased
- Address comments
libcxx/include/__algorithm/ranges_mismatch.h | ||
---|---|---|
48 | 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. | |
libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp | ||
17–31 | Veto on doing this for test_ranges. For that I would either have to inline the return type check as well or explicitly name the template parameters. Both of which make the code definitely less readable. |
Just test nits at this point (probably all related to inlining test_ranges :))
libcxx/include/__algorithm/ranges_mismatch.h | ||
---|---|---|
37 | 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. | |
39–46 | Just a note to say this implementation LGTM now, and awesome, I see that testing with cpp20_input_iterator caught a bug :) | |
48 | Yikes, I see this now: https://godbolt.org/z/WPzrhda1f Is that the problem you mean? If so, OK, no-regression-test is fine. | |
libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp | ||
24–28 | And change the signature on line 19 to take int *expected1, int *expected2. | |
89 | 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). 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. |
- Updates
libcxx/include/__algorithm/ranges_mismatch.h | ||
---|---|---|
48 | 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. |
LGTM % nits.
libcxx/include/__algorithm/ranges_mismatch.h | ||
---|---|---|
31 | 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. | |
48 | 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. | |
libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp | ||
66 | a.begin() + 3 is a funny way to spell a.end(). Please also test when the first range is shorter, i.e. add test_iterators(b, b + 2, a, a + 3, b + 2, a + 2). | |
168 | I claim that we traditionally put a blank line before the return 0;. |
libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp | ||
---|---|---|
13 | Please document what you're testing by adding the synopsis here. That's what we do everywhere else. | |
22 | You could easily inline that function into the only place where it's called (unless I missed a second usage?). | |
38 | Please inline those test_foo() functions into the body below, and separate them with scopes like we've started doing: // meaningful comment { <...> } Using a comment allows for something more descriptive than just test_sentinel. |
- Rebased
- Address comments
libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp | ||
---|---|---|
22 | It's also used in test_different_lengths(). |
libcxx/include/__algorithm/ranges_mismatch.h | ||
---|---|---|
62 | Any reason why we don't implement the algorithm here and then call *this in the range version immediately below? | |
libcxx/include/algorithm | ||
30–41 | This is strangely formatted. Also, we need to say // since C++20. | |
libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp | ||
107 | We should add test with non-default projections and predicate for the range overload. Edit: And for the iterator overload too, since it appears we don't have such tests. |
- Address comments
libcxx/include/__algorithm/ranges_mismatch.h | ||
---|---|---|
62 | With the current implementation the predicates and projections are never moved. using *this() would mean we have to move them at least once. |
LGTM with my comments addressed. Thanks!
libcxx/include/__algorithm/ranges_mismatch.h | ||
---|---|---|
62 | Oh, I see. Clever. Can you please add a libc++ specific test for that? I know it's nitpicky, but that seems like a useful guarantee to test if we're providing it. I suggest finding a simple way to test this because I'll be requesting the same kind of test for other algorithms as well (e.g. find_if). | |
libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp | ||
129 |
By the way, I know CI is flaky right now with failures around installing cxxabi_config.h -- I'm getting there but there's a couple pre-requisites to fix this. For now, please ignore.
libcxx/include/__algorithm/ranges_mismatch.h | ||
---|---|---|
62 | I'd be happy to tackle this in a different PR; it'd basically be a Ranges version of libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp. |
libcxx/docs/Status/RangesAlgorithms.csv | ||
---|---|---|
10 | Ultranit: can you use the checkmark emoji instead? It would be consistent with other completed entries (and I think stands out better visually). | |
libcxx/include/__algorithm/ranges_mismatch.h | ||
31 | Since D118736 has landed, does this need to change again? | |
82 | This comment looks like it doesn't match the #if condition. | |
libcxx/include/algorithm | ||
34 | Do you need to also add mismatch_result to the synopsis? | |
libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp | ||
36 | Ultranit: include <utility>? | |
43 | Nit: it's not obvious from names alone how test_iters is different from test_iterators. Feel free to ignore because the scope is small enough. (For a concrete suggestion, I'd use test_iters and test_iters_impl, though it's certainly not perfect) | |
108 | Very optional: you could use CTAD here to avoid providing an element count. | |
124 | Question: is wrapping in Iter necessary? It seems like array-to-pointer decay would work. | |
129 | Nit: s/sized/size/? | |
172 | Optional: this seems like a test of std::ranges::in_in_result? | |
290 | I think this patch also needs to test the constraints to make sure that parameters that don't satisfy input_iterator, indirectly_comparable, etc. SFINAE away properly. |
@philnik Sorry, looks like I'm a little late with my review. Can you please address some of the comments in a follow-up? You can definitely ignore the nits and optionals -- I'm mostly concerned about the potentially missing tests for SFINAE and missing mismatch_result from the synopsis. Thanks!
Ultranit: can you use the checkmark emoji instead? It would be consistent with other completed entries (and I think stands out better visually).