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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

Looking great so far, thank you for working on this!



================
Comment at: libcxx/include/__ranges/split_view.h:59
+  using _Cache                                     = __non_propagating_cache<subrange<iterator_t<_View>>>;
+  _LIBCPP_NO_UNIQUE_ADDRESS _Cache __cached_begin_ = _Cache();
+
----------------
huixie90 wrote:
> pointless `_LIBCPP_NO_UNIQUE_ADDRESS` . 
Can we omit it?


================
Comment at: libcxx/include/__ranges/split_view.h:90
+                 constructible_from<_Pattern, single_view<range_value_t<_Range>>>
+  _LIBCPP_HIDE_FROM_ABI constexpr split_view(_Range&& __range, range_value_t<_Range> __ele)
+      : __base_(views::all(std::forward<_Range>(__range))), __pattern_(views::single(std::move(__ele))) {}
----------------
Nit: `s/ele/elem/` (or `el` if you prefer a shorter name).


================
Comment at: libcxx/include/__ranges/split_view.h:120
+private:
+  bool __trailing_empty_                                        = false;
+  split_view<_View, _Pattern>* __parent_                        = nullptr;
----------------
philnik wrote:
> Having this first seems like a bad idea. On 64-bit platforms this results in 7 bytes of padding. Maybe it would make sense to bit-pack this into the `split_view<_View, _Pattern>*`?
Also I don't think we have to follow the order in the standard, but there it's the last member variable.


================
Comment at: libcxx/include/__ranges/split_view.h:200
+  template <class _Range, class _Pattern>
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
+  constexpr auto operator()(_Range&& __range, _Pattern&& __pattern) const
----------------
philnik wrote:
> same for the other one.
@philnik We've been doing it this way for many (perhaps all) other views, should we change the others as well?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/begin.pass.cpp:151
+    auto calledTimesAfterFirstBegin = equalsCalledTimes;
+    assert(calledTimesAfterFirstBegin != 0);
+
----------------
Question: is it not guaranteed to be `1`?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/begin.pass.cpp:169
+
+void testBeginCached() {}
+
----------------
I'll just leave a comment here to make sure we don't forget to revisit this test later (I presume it's a TODO).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/ctor.range.pass.cpp:30
+
+  constexpr ElementWithCounting(const ElementWithCounting& rhs)
+      : times_copied(rhs.times_copied), times_moved(rhs.times_moved) {
----------------
Optional: can counting be refactored out into a base class that is used as a mixin for `RangeWithCounting`?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/end.pass.cpp:39
+
+    std::ranges::split_view sv(buffer, 3);
+    using SplitIter                           = std::ranges::iterator_t<decltype(sv)>;
----------------
Nit: can you use a more visible number for splitting, e.g. `0` or `-1`? `3` doesn't really stand out.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/end.pass.cpp:55
+    std::ranges::split_view sv(range, 3);
+    auto st = sv.end();
+
----------------
Nit: I'm not sure what `st` stands for, can you consider a more descriptive name?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/general.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Would it be possible to reuse tests from `/range.adaptors/range.lazy.split/general.pass.cpp`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142063