[libcxx-commits] [PATCH] D151274: [libc++] Optimize for_each for segmented iterators

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I think this will be LGTM after comments, but I'd like to take a quick look again.



================
Comment at: libcxx/docs/ReleaseNotes/18.rst:60-61
 
+- ``std::for_each`` has been optimized for segmented iterators in C++23 and
+  later, which can lead up to 40x performance improvements.
+
----------------



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/for_each.pass.cpp:12
+// template<InputIterator Iter, Callable<auto, Iter::reference> Function>
+//   requires CopyConstructible<Function>
+//   constexpr Function   // constexpr after C++17
----------------
This seems wrong? It was there before, but it still seems incorrect.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/for_each.pass.cpp:13
+//   requires CopyConstructible<Function>
+//   constexpr Function   // constexpr after C++17
+//   for_each(Iter first, Iter last, Function f);
----------------



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/for_each.pass.cpp:40
+TEST_CONSTEXPR_CXX20 bool test() {
+  int ia[]         = {0, 1, 2, 3, 4, 5};
+  const unsigned s = sizeof(ia) / sizeof(ia[0]);
----------------
We need tests with 0 and 1 length sequences at the very least.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/for_each.pass.cpp:68-72
+  // check that segmented iterators work properly
+  std::deque<int> d(50);
+  int index = 0;
+
+  std::for_each(d.begin(), d.end(), deque_test(d, index));
----------------
Same here, we should test a couple of different sizes of deque, in particular around the size of underlying segments. And 0 and 1.


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:12
 # temporary the ToT version is preferred.
-find_package(Clang 17)
+find_package(Clang 18)
 if (NOT Clang_FOUND)
----------------
I don't think this should be part of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151274