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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

The implementation looks mostly good. I didn't look at the tests yet.



================
Comment at: libcxx/include/__ranges/split_view.h:120
+private:
+  bool __trailing_empty_                                        = false;
+  split_view<_View, _Pattern>* __parent_                        = nullptr;
----------------
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>*`?


================
Comment at: libcxx/include/__ranges/split_view.h:175
+private:
+  _LIBCPP_NO_UNIQUE_ADDRESS sentinel_t<_View> __end_ = sentinel_t<_View>(); // exposition only
+
----------------
Why the comment here specifically, but not anywhere else?


================
Comment at: libcxx/include/__ranges/split_view.h:189-193
+template <class _Range, class _Pattern>
+split_view(_Range&&, _Pattern&&) -> split_view<views::all_t<_Range>, views::all_t<_Pattern>>;
+
+template <forward_range _Range>
+split_view(_Range&&, range_value_t<_Range>) -> split_view<views::all_t<_Range>, single_view<range_value_t<_Range>>>;
----------------
Can you move this directly below the `split_view`, like we normally do?


================
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
----------------
same for the other one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142063