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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:10
 Search,adjacent_find,Not assigned,n/a,Not started
-Search,mismatch,Not assigned,n/a,Not started
+Search,mismatch,Nikolas Klauser,`D117817 <https://llvm.org/D117817>`,Complete
 Search,equal,Not assigned,n/a,Not started
----------------
Ultranit: can you use the checkmark emoji instead? It would be consistent with other completed entries (and I think stands out better visually).


================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:81
+
+#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
This comment looks like it doesn't match the `#if` condition.


================
Comment at: libcxx/include/__algorithm/ranges_mismatch.h:30
+
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
Quuxplusone wrote:
> 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.
Since D118736 has landed, does this need to change again?


================
Comment at: libcxx/include/algorithm:51
+    requires indirectly_comparable<I1, I2, Pred, Proj1, Proj2>
+  constexpr mismatch_result<_I1, _I2>
+  mismatch()(I1 first1, S1 last1, I2 first2, S2 last2, Pred pred = {}, Proj1 proj1 = {}, Proj2 proj2 = {}) // since C++20
----------------
Do you need to also add `mismatch_result` to the synopsis?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:35
+  using Expected = std::ranges::mismatch_result<Iter1, Iter2>;
+  std::same_as<Expected> auto ret = std::ranges::mismatch(std::move(begin1), sentinel_wrapper<Iter1>(std::move(end1)),
+                                                          std::move(begin2), sentinel_wrapper<Iter2>(std::move(end2)));
----------------
Ultranit: include `<utility>`?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:42
+template <class Iter1, class Iter2>
+constexpr void test_iters() {
+  int a[] = {1, 2, 3, 4, 5};
----------------
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)


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:107
+  { // test with a range
+    std::array<int, 5> a = {1, 2, 3, 4, 5};
+    std::array<int, 5> b = {1, 2, 3, 5, 4};
----------------
Very optional: you could use CTAD here to avoid providing an element count.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:123
+
+    std::same_as<Expected> auto r = std::ranges::mismatch(Iter(a), Sentinel(a + 5), Iter(b), Sentinel(b + 5));
+    assert(r.in1 == a + 3);
----------------
Question: is wrapping in `Iter` necessary? It seems like array-to-pointer decay would work.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:128
+
+  { // test with different array sized
+    {
----------------
Nit: `s/sized/size/`?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:171
+
+  { // test structured bindings
+    int a[] = {1, 2, 3, 4};
----------------
Optional: this seems like a test of `std::ranges::in_in_result`?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp:289
+
+int main(int, char**) {
+  test();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117817