[libcxx-commits] [PATCH] D142063: [libc++][ranges] implement `std::ranges::split_view` (targeting llvm 16)

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added a comment.
This revision is now accepted and ready to land.

Thanks a lot for working on this! LGTM with just a few comments.

(two more important comments are to fix a stray `lazy_split` and update the release notes (for release notes it's fine to do a follow-up). The rest is optional)



================
Comment at: libcxx/docs/Status/Cxx20Papers.csv:197
 "`P2325R3 <https://wg21.link/P2325R3>`__","LWG","Views should not be required to be default constructible","June 2021","|Complete|","16.0","|ranges|"
-"`P2210R2 <https://wg21.link/P2210R2>`__","LWG","Superior String Splitting","June 2021","|In progress|","","|ranges|"
+"`P2210R2 <https://wg21.link/P2210R2>`__","LWG","Superior String Splitting","June 2021","|Complete|","16.0","|ranges|"
 "`P2216R3 <https://wg21.link/P2216R3>`__","LWG","std::format improvements","June 2021","|Complete|","15.0"
----------------
This should also be mentioned in the release notes. It might be easier to do that in a follow-up.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/general.pass.cpp:45
+  auto expected_it = expected.begin();
+  for (auto e : input | std::ranges::views::lazy_split(separator)) {
+    if (expected_it == expected.end())
----------------
`s/lazy_split/split/`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/iterator/increment.pass.cpp:27
+  {
+    int buffer[] = {0, 1, 2, -1, 4, -1, 6};
+    auto input   = make_subrange(buffer);
----------------
Optional: maybe make one of the next ranges have more characters than one?



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/iterator/increment.pass.cpp:27
+  {
+    int buffer[] = {0, 1, 2, -1, 4, -1, 6};
+    auto input   = make_subrange(buffer);
----------------
var-const wrote:
> Optional: maybe make one of the next ranges have more characters than one?
> 
Do we need to also check for cases where there are two or more separators in a row and when the input is empty? If it's covered by `general.pass.cpp`, that's fine.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/iterator/increment.pass.cpp:37
+      decltype(auto) it1 = ++it;
+      static_assert(std::is_same_v<decltype(it1), SplitIter&>);
+      assert(&it1 == &it);
----------------
Nit: any reason not to use `same_as` in the statement above? (If it's a compiler bug or something like that, never mind)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/iterator/member_types.compile.pass.cpp:27
+constexpr void testIteratorTypedef() {
+  static_assert(std::same_as<typename SplitIter<Range<Iter>, Range<PatternIter>>::iterator_concept, //
+                             std::forward_iterator_tag>);
----------------
Optional: a typedef for `SplitIter<Range<Iter>, Range<PatternIter>` would cut a little boilerplate here. If you prefer having this more explicit, that's totally fine too.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/sentinel/equal.pass.cpp:36
+      assert(!(b == e));
+      assert(b != e);
+    }
----------------
Optional: maybe advance the iterator to also check the case when it equals the sentinel?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142063