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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Almost LGTM, just some nits.



================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:8
 Search,find_if_not,Nikolas Klauser,`D121248 <https://reviews.llvm.org/D121248>`_,✅
-Search,find_first_of,Not assigned,n/a,Not started
+Search,find_first_of,Nikolas Klauser,` <https://reviews.llvm.org/>`_,✅
 Search,adjacent_find,Not assigned,n/a,Not started
----------------
Nit: please update the link. Also, I don't think there should be a space between the tilde and the `<` character.


================
Comment at: libcxx/include/__algorithm/ranges_find_first_of.h:43
+    for (; __first1 != __last1; ++__first1) {
+      for (auto __j = __first2; __j != __last2; ++__j) {
+        if (std::invoke(__pred, std::invoke(__proj1, *__first1), std::invoke(__proj2, *__j)))
----------------
Why are we creating a new variable instead of incrementing `__first2`, which would be consistent with `__first1`?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:97
+  // simple test
+  test<Iter1, Sent1, Iter2, Sent2, 4, 2>({.input1 = {1, 2, 3, 4}, .input2 = {2, 3}, .expected = 1});
+  // other elements from input2 are checked
----------------
(By the way, I like these "named arguments")


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:106
+  test<Iter1, Sent1, Iter2, Sent2, 0, 0>({.input1 = {}, .input2 = {}, .expected = 0});
+  // the first element is checked properly
+  test<Iter1, Sent1, Iter2, Sent2, 5, 2>({.input1 = {5, 4, 3, 2, 1}, .input2 = {1, 5}, .expected = 0});
----------------
For exhaustiveness, I'd also add a test case when `input2` consists of a single element (currently, all the successful cases have `input2` with several elements).


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:122
+  test_iterators<Iter1, Sent1, int*>();
+
+}
----------------
Nit: unnecessary blank line.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:170
+
+  { // check that std::invoke is used
+    struct S1 {
----------------
Great test, can you please check if there are any other range algorithm tests where a similar test would be valuable?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:182
+
+    {
+      S1 a[] = {1, 2, 3, 4};
----------------
Nit: swap the order, so that the iterator overload goes before the range overload?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:221
+      assert(ret == a + 4);
+      assert(predCount <= 8);
+      assert(proj1Count <= 8);
----------------
Question: perhaps check for the exact value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126529