[libcxx-commits] [PATCH] D124332: [libc++] Implement ranges::for_each{, _n}

[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.

Looking pretty good, just a few comments.



================
Comment at: libcxx/include/__algorithm/ranges_for_each_n.h:51
+    }
+    return {std::move(__first), __func};
+  }
----------------
Nit: move `__func` as well?


================
Comment at: libcxx/include/algorithm:967
 #include <__algorithm/ranges_find_if_not.h>
+#include <__algorithm/ranges_for_each.h>
+#include <__algorithm/ranges_for_each_n.h>
----------------
Please update the synopsis as well.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/ranges.for_each.pass.cpp:11
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+
----------------
Please add the synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/ranges.for_each.pass.cpp:31
+static_assert(!HasForEachIt<int*, SentinelForNotSemiregular>);
+static_assert(!HasForEachIt<int*, SentinelForNotWeaklyEqualityComparableWith>);
+
----------------
Please also check the `indirectly_unary_invocable` constraint.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/ranges.for_each.pass.cpp:45
+constexpr void test_iterator() {
+  { // simple test
+    {
----------------
Please also check with an empty range.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/ranges.for_each.pass.cpp:49
+      int a[] = {1, 6, 3, 4};
+      std::same_as<std::ranges::for_each_result<Iter, decltype(func)>> auto ret =
+          std::ranges::for_each(Iter(a), Sent(Iter(a + 4)), func);
----------------
Nit: `decltype(auto)`.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/ranges.for_each.pass.cpp:57
+      int i = 0;
+      ret.fun(i);
+      assert(i == 4);
----------------
Can you also check that it works when the functor is move-only (since the standard mandates that `for_each_result` should initialize the functor by moving)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124332