[libcxx-commits] [PATCH] D126529: [libc++] Implement ranges::find_first_of

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good modulo some minor issues. But before approving I like to know whether or not we want to avoid code duplication between the "old" algorithms and their ranges based equivalents.



================
Comment at: libcxx/include/__algorithm/ranges_find_first_of.h:42
+                              _Proj2& __proj2) {
+    for (; __first1 != __last1; ++__first1) {
+      for (auto __j = __first2; __j != __last2; ++__j) {
----------------
I saw @ldionne recently raised some concerns regarding duplicating algorithms for the ranges and non-ranges version. Have we found a solution for that concern?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:75
+  std::array<int, N2> input2;
+  int expected;
+};
----------------
I find it a bit confusion the have an `array<int, x>` and an `int expected` not related.
Since the `expected` is an offset giving it a better type helps to avoid the confusion for me.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:101
+  // other elements from input2 are checked
+  test<Iter1, Sent1, Iter2, Sent2, 4, 2>({.input1 = {1, 2, 3, 4}, .input2 = {3, 2}, .expected = 1});
+  // an empty second range returns last
----------------
This is a duplicate of line 99.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:112
+  test<Iter1, Sent1, Iter2, Sent2, 5, 2>({.input1 = {5, 4, 3, 2, 1}, .input2 = {1, 6}, .expected = 4});
+}
+
----------------
I would like to see two non-empty ranges that have no match at all. For example
`test<Iter1, Sent1, Iter2, Sent2, 4, 4>({.input1 = {1, 3, 5, 7}, .input2 = {0, 2, 4, 6}, .expected = 5});`


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:147
+    {
+      auto ret = std::ranges::find_first_of(a, a + 4, b, b + 1, std::ranges::greater{});
+      assert(ret == a + 2);
----------------
For clarity I would prefer to use `std::begin()` and `std::end()` here.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:207
+
+  { // check that the complexity rewuirements are met
+    int a[] = {1, 2, 3, 4};
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126529