Fix review comments

Author: winner245

libcxx/docs/ReleaseNotes/21.rst

@@ -61,8 +61,8 @@ Improvements and New Features
 - Updated formatting library to Unicode 16.0.0.
 
 - The ``std::ranges::for_each`` and ``std::ranges::for_each_n`` algorithms have been optimized for segmented iterators,
-  resulting in performance improvements of up to 21.3x for ``std::deque::iterator`` segmented inputs and 24.9x for
-  ``join_view`` of ``vector<vector<T>>``.
+  resulting in performance improvements of up to 21.3x for ``std::deque::iterator`` and 24.9x for ``join_view`` of
+  ``vector<vector<char>>``.
 
 Deprecations and Removals
 -------------------------

libcxx/include/__algorithm/for_each.h

@@ -12,6 +12,8 @@
 
 #include <__algorithm/for_each_segment.h>
 #include <__config>
+#include <__functional/identity.h>
+#include <__functional/invoke.h>
 #include <__iterator/segmented_iterator.h>
 #include <__type_traits/enable_if.h>
 #include <__utility/move.h>
@@ -25,44 +27,49 @@ _LIBCPP_PUSH_MACROS
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-template <class _InputIterator, class _Sent, class _Function>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Function
-__for_each(_InputIterator __first, _Sent __last, _Function& __f) {
+template <class _InputIterator, class _Sent, class _Function, class _Proj>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _InputIterator
+__for_each(_InputIterator __first, _Sent __last, _Function& __f, _Proj& __proj) {
   for (; __first != __last; ++__first)
-    __f(*__first);
-  return std::move(__f);
+    std::invoke(__f, std::invoke(__proj, *__first));
+  return std::move(__first);
 }
 
 // __segment_processor handles the per-segment processing by applying the user-provided function to each element
 // within the segment. It acts as a functor passed to the segmented iterator algorithm __for_each_segment.
-template <class _SegmentedIterator, class _Function>
+template <class _SegmentedIterator, class _Function, class _Proj>
 struct __segment_processor {
   using _Traits _LIBCPP_NODEBUG = __segmented_iterator_traits<_SegmentedIterator>;
 
   _Function& __func_;
+  _Proj& __proj_;
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 explicit __segment_processor(_Function& __func)
-      : __func_(__func) {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 explicit __segment_processor(_Function& __func, _Proj& __proj)
+      : __func_(__func), __proj_(__proj) {}
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
   operator()(typename _Traits::__local_iterator __lfirst, typename _Traits::__local_iterator __llast) {
-    std::__for_each(__lfirst, __llast, __func_);
+    std::__for_each(__lfirst, __llast, __func_, __proj_);
   }
 };
 
 template <class _SegmentedIterator,
           class _Function,
+          class _Proj,
           __enable_if_t<__is_segmented_iterator<_SegmentedIterator>::value, int> = 0>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Function
-__for_each(_SegmentedIterator __first, _SegmentedIterator __last, _Function& __func) {
-  std::__for_each_segment(__first, __last, std::__segment_processor<_SegmentedIterator, _Function>(__func));
-  return std::move(__func);
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _SegmentedIterator
+__for_each(_SegmentedIterator __first, _SegmentedIterator __last, _Function& __func, _Proj& __proj) {
+  std::__for_each_segment(
+      __first, __last, std::__segment_processor<_SegmentedIterator, _Function, _Proj>(__func, __proj));
+  return std::move(__last);
 }
 
 template <class _InputIterator, class _Function>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Function
 for_each(_InputIterator __first, _InputIterator __last, _Function __f) {
-  return std::__for_each(__first, __last, __f);
+  __identity __proj;
+  std::__for_each(__first, __last, __f, __proj);
+  return std::move(__f);
 }
 
 _LIBCPP_END_NAMESPACE_STD

libcxx/include/__algorithm/for_each_n.h

@@ -13,8 +13,9 @@
 #include <__algorithm/for_each.h>
 #include <__algorithm/for_each_n_segment.h>
 #include <__config>
+#include <__functional/identity.h>
+#include <__functional/invoke.h>
 #include <__iterator/iterator_traits.h>
-#include <__iterator/next.h>
 #include <__iterator/segmented_iterator.h>
 #include <__type_traits/enable_if.h>
 #include <__utility/convert_to_integral.h>
@@ -25,35 +26,63 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-#if _LIBCPP_STD_VER >= 17
-
 template <class _InputIterator,
           class _Size,
           class _Function,
-          __enable_if_t<!__is_segmented_iterator<_InputIterator>::value ||
-                            __has_exactly_input_iterator_category<_InputIterator>::value,
+          class _Proj,
+          __enable_if_t<!__has_random_access_iterator_category<_InputIterator>::value &&
+                            (!__is_segmented_iterator<_InputIterator>::value
+                             //   || !__has_random_access_iterator_category<
+                             //      typename __segmented_iterator_traits<_InputIterator>::__local_iterator>::value
+                             ), // TODO: __segmented_iterator_traits<_InputIterator> results in template instantiation
+                                // during SFINAE, which is a hard error to be fixed. Once fixed, we should uncomment.
                         int> = 0>
 inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _InputIterator
-for_each_n(_InputIterator __first, _Size __orig_n, _Function __f) {
+__for_each_n(_InputIterator __first, _Size __orig_n, _Function& __f, _Proj& __proj) {
   typedef decltype(std::__convert_to_integral(__orig_n)) _IntegralSize;
   _IntegralSize __n = __orig_n;
   while (__n > 0) {
-    __f(*__first);
+    std::invoke(__f, std::invoke(__proj, *__first));
     ++__first;
     --__n;
   }
   return __first;
 }
 
+template <class _RandIter,
+          class _Size,
+          class _Function,
+          class _Proj,
+          __enable_if_t<__has_random_access_iterator_category<_RandIter>::value, int> = 0>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _RandIter
+__for_each_n(_RandIter __first, _Size __orig_n, _Function& __f, _Proj& __proj) {
+  typedef decltype(std::__convert_to_integral(__orig_n)) _IntegralSize;
+  _IntegralSize __n = __orig_n;
+  return std::__for_each(__first, __first + __n, __f, __proj);
+}
+
 template <class _SegmentedIterator,
           class _Size,
           class _Function,
-          __enable_if_t<__is_segmented_iterator<_SegmentedIterator>::value &&
-                            __has_forward_iterator_category<_SegmentedIterator>::value,
+          class _Proj,
+          __enable_if_t<!__has_random_access_iterator_category<_SegmentedIterator>::value &&
+                            __is_segmented_iterator<_SegmentedIterator>::value &&
+                            __has_random_access_iterator_category<
+                                typename __segmented_iterator_traits<_SegmentedIterator>::__local_iterator>::value,
                         int> = 0>
 inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _SegmentedIterator
-for_each_n(_SegmentedIterator __first, _Size __orig_n, _Function __f) {
-  return std::__for_each_n_segment(__first, __orig_n, std::__segment_processor<_SegmentedIterator, _Function>(__f));
+__for_each_n(_SegmentedIterator __first, _Size __orig_n, _Function& __f, _Proj& __proj) {
+  return std::__for_each_n_segment(
+      __first, __orig_n, std::__segment_processor<_SegmentedIterator, _Function, _Proj>(__f, __proj));
+}
+
+#if _LIBCPP_STD_VER >= 17
+
+template <class _InputIterator, class _Size, class _Function>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _InputIterator
+for_each_n(_InputIterator __first, _Size __orig_n, _Function __f) {
+  __identity __proj;
+  return std::__for_each_n(__first, __orig_n, __f, __proj);
 }
 
 #endif

libcxx/include/__algorithm/for_each_n_segment.h

@@ -11,6 +11,7 @@
 
 #include <__config>
 #include <__iterator/distance.h>
+#include <__iterator/iterator_traits.h>
 #include <__iterator/next.h>
 #include <__iterator/segmented_iterator.h>
 #include <__utility/convert_to_integral.h>
@@ -22,14 +23,18 @@
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // __for_each_n_segment optimizes linear iteration over segmented iterators. It processes a segmented
-// input range defined by (__first, __orig_n), where __first is the starting segmented iterator and
-// __orig_n is the number of elements to process. The functor __func is applied to each segment using
+// input range defined by [__first, __first + __n), where __first is the starting segmented iterator
+// and __n is the number of elements to process. The functor __func is applied to each segment using
 // local iterator pairs for that segment. The return value of __func is ignored, and the function
 // returns an iterator pointing to one past the last processed element in the input range.
 
 template <class _SegmentedIterator, class _Size, class _Functor>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _SegmentedIterator
 __for_each_n_segment(_SegmentedIterator __first, _Size __orig_n, _Functor __func) {
+  static_assert(__is_segmented_iterator<_SegmentedIterator>::value &&
+                    __has_random_access_iterator_category<
+                        typename __segmented_iterator_traits<_SegmentedIterator>::__local_iterator>::value,
+                "__for_each_n_segment only works with segmented iterators with random-access local iterators");
   if (__orig_n == 0)
     return __first;
 

libcxx/include/__algorithm/ranges_for_each.h

@@ -44,19 +44,13 @@ struct __for_each {
   template <class _Iter, class _Sent, class _Proj, class _Func>
   _LIBCPP_HIDE_FROM_ABI constexpr static for_each_result<_Iter, _Func>
   __for_each_impl(_Iter __first, _Sent __last, _Func& __func, _Proj& __proj) {
-    if constexpr (std::assignable_from<_Iter&, _Sent>) {
-      _Iter __end = std::move(__last);
-      std::for_each(__first, __end, [&](auto&& __val) { std::invoke(__func, std::invoke(__proj, __val)); });
-      return {std::move(__end), std::move(__func)};
-    } else if constexpr (sized_sentinel_for<_Sent, _Iter>) {
-      auto __end = std::for_each_n(__first, __last - __first, [&](auto&& __val) {
-        std::invoke(__func, std::invoke(__proj, __val));
-      });
+    if constexpr (!std::assignable_from<_Iter&, _Sent> && sized_sentinel_for<_Sent, _Iter>) {
+      auto __n   = __last - __first;
+      auto __end = std::__for_each_n(std::move(__first), __n, __func, __proj);
       return {std::move(__end), std::move(__func)};
     } else {
-      for (; __first != __last; ++__first)
-        std::invoke(__func, std::invoke(__proj, *__first));
-      return {std::move(__first), std::move(__func)};
+      auto __end = std::__for_each(std::move(__first), std::move(__last), __func, __proj);
+      return {std::move(__end), std::move(__func)};
     }
   }
 

libcxx/include/__algorithm/ranges_for_each_n.h

@@ -17,7 +17,6 @@
 #include <__iterator/concepts.h>
 #include <__iterator/incrementable_traits.h>
 #include <__iterator/iterator_traits.h>
-#include <__iterator/next.h>
 #include <__iterator/projected.h>
 #include <__ranges/concepts.h>
 #include <__utility/move.h>
@@ -42,17 +41,8 @@ struct __for_each_n {
   template <input_iterator _Iter, class _Proj = identity, indirectly_unary_invocable<projected<_Iter, _Proj>> _Func>
   _LIBCPP_HIDE_FROM_ABI constexpr for_each_n_result<_Iter, _Func>
   operator()(_Iter __first, iter_difference_t<_Iter> __count, _Func __func, _Proj __proj = {}) const {
-    if constexpr (forward_iterator<_Iter>) {
-      auto __f    = [&](auto&& __val) { std::invoke(__func, std::invoke(__proj, __val)); };
-      auto __last = std::for_each_n(__first, __count, __f);
-      return {std::move(__last), std::move(__func)};
-    } else {
-      while (__count-- > 0) {
-        std::invoke(__func, std::invoke(__proj, *__first));
-        ++__first;
-      }
-      return {std::move(__first), std::move(__func)};
-    }
+    auto __last = std::__for_each_n(std::move(__first), __count, __func, __proj);
+    return {std::move(__last), std::move(__func)};
   }
 };
 

libcxx/test/benchmarks/algorithms/nonmodifying/for_each.bench.cpp

@@ -12,6 +12,7 @@
 #include <cstddef>
 #include <deque>
 #include <list>
+#include <ranges>
 #include <string>
 #include <vector>
 
@@ -48,20 +49,6 @@ int main(int argc, char** argv) {
           ->Arg(1 << 16)
           ->Arg(1 << 18);
     };
-    bm.operator()<std::vector<char>>("std::for_each(vector<char>)", std_for_each);
-    bm.operator()<std::deque<char>>("std::for_each(deque<char>)", std_for_each);
-    bm.operator()<std::list<char>>("std::for_each(list<char>)", std_for_each);
-    bm.operator()<std::vector<char>>("rng::for_each(vector<char>)", std::ranges::for_each);
-    bm.operator()<std::deque<char>>("rng::for_each(deque<char>)", std::ranges::for_each);
-    bm.operator()<std::list<char>>("rng::for_each(list<char>)", std::ranges::for_each);
-
-    bm.operator()<std::vector<short>>("std::for_each(vector<short>)", std_for_each);
-    bm.operator()<std::deque<short>>("std::for_each(deque<short>)", std_for_each);
-    bm.operator()<std::list<short>>("std::for_each(list<short>)", std_for_each);
-    bm.operator()<std::vector<short>>("rng::for_each(vector<short>)", std::ranges::for_each);
-    bm.operator()<std::deque<short>>("rng::for_each(deque<short>)", std::ranges::for_each);
-    bm.operator()<std::list<short>>("rng::for_each(list<short>)", std::ranges::for_each);
-
     bm.operator()<std::vector<int>>("std::for_each(vector<int>)", std_for_each);
     bm.operator()<std::deque<int>>("std::for_each(deque<int>)", std_for_each);
     bm.operator()<std::list<int>>("std::for_each(list<int>)", std_for_each);
@@ -70,6 +57,47 @@ int main(int argc, char** argv) {
     bm.operator()<std::list<int>>("rng::for_each(list<int>)", std::ranges::for_each);
   }
 
+  // {std,ranges}::for_each for join_view
+  {
+    auto bm = []<class Container>(std::string name, auto for_each) {
+      using C1       = typename Container::value_type;
+      using ElemType = typename C1::value_type;
+
+      benchmark::RegisterBenchmark(
+          name,
+          [for_each](auto& st) {
+            std::size_t const size     = st.range(0);
+            std::size_t const seg_size = 256;
+            std::size_t const segments = (size + seg_size - 1) / seg_size;
+            Container c(segments);
+            for (std::size_t i = 0, n = size; i < segments; ++i, n -= seg_size) {
+              c[i].resize(std::min(seg_size, n), ElemType(1));
+            }
+
+            auto view  = c | std::views::join;
+            auto first = view.begin();
+            auto last  = view.end();
+
+            for ([[maybe_unused]] auto _ : st) {
+              benchmark::DoNotOptimize(c);
+              auto result = for_each(first, last, [](ElemType& x) { x = std::clamp<ElemType>(x, 10, 100); });
+              benchmark::DoNotOptimize(result);
+            }
+          })
+          ->Arg(8)
+          ->Arg(32)
+          ->Arg(50) // non power-of-two
+          ->Arg(1024)
+          ->Arg(4096)
+          ->Arg(8192)
+          ->Arg(1 << 14)
+          ->Arg(1 << 16)
+          ->Arg(1 << 18);
+    };
+    bm.operator()<std::vector<std::vector<int>>>("std::for_each(join_view(vector<vector<int>>))", std_for_each);
+    bm.operator()<std::vector<std::vector<int>>>("rng::for_each(join_view(vector<vector<int>>)", std::ranges::for_each);
+  }
+
   benchmark::Initialize(&argc, argv);
   benchmark::RunSpecifiedBenchmarks();
   benchmark::Shutdown();