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

Fri Jan 20 10:05:19 PST 2023

ldionne accepted this revision as: ldionne.
ldionne added a comment.

This looks reasonable to me. This is missing tests for iterators though. I'll let @var-const give the final approval.

================
Comment at: libcxx/docs/Status/Cxx2bIssues.csv:134
 `3589 <https://wg21.link/LWG3589>`__,"The ``const`` lvalue reference overload of ``get`` for ``subrange`` does not constrain ``I`` to be ``copyable`` when ``N == 0``","October 2021","|Complete|","14.0","|ranges|"
-`3590 <https://wg21.link/LWG3590>`__,"``split_view::base() const &`` is overconstrained","October 2021","","","|ranges|"
-`3591 <https://wg21.link/LWG3591>`__,"``lazy_split_view<input_view>::inner-iterator::base() &&`` invalidates outer iterators","October 2021","","","|ranges|"
-`3592 <https://wg21.link/LWG3592>`__,"``lazy_split_view`` needs to check the simpleness of Pattern","October 2021","","","|ranges|"
-`3593 <https://wg21.link/LWG3593>`__,"Several iterators' ``base() const &`` and ``lazy_split_view::outer-iterator::value_type::end()`` missing ``noexcept``","October 2021","","","|ranges|"
+`3590 <https://wg21.link/LWG3590>`__,"``split_view::base() const &`` is overconstrained","October 2021","|Complete|","16.0","|ranges|"
+`3591 <https://wg21.link/LWG3591>`__,"``lazy_split_view<input_view>::inner-iterator::base() &&`` invalidates outer iterators","October 2021","|Complete|","16.0","|ranges|"
----------------
For each issue being marked as completed, can you please make sure that we have a regression test? I don't mind if that means you don't mark them as completed in this patch, but I would like to avoid marking something as completed if we don't have a test for it.

================
Comment at: libcxx/include/__ranges/split_view.h:120
+
+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 remove this constraint and see if anything fails? If not, it means we're missing test coverage. In particular, if you have a Pattern that would both work with `all_t` and is also the `range_value_t` of your range, I guess we want to take this deduction guide, not the one above.

================
Comment at: libcxx/include/__ranges/split_view.h:120
+private:
+  bool __trailing_empty_                                        = false;
+  split_view<_View, _Pattern>* __parent_                        = nullptr;
----------------
philnik wrote:
> huixie90 wrote:
> > var-const wrote:
> > > 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.
> > Yes I will move the bool to the end
> > Could you please elaborate on bit-packing? 
> We can make sure that `split_view` is always at least 2 byte aligned (which it is in most cases anyways), which guarantees that the low bit of the pointer is always zero. Since we know that the low bit is always zero, we can use it as a flag. We just have to bit-and the pointer if we want to dereference it. I have an implementation of a utility class for this purpose in D140278, but that's currently not `constexpr`-friendly and needs some more work in general. Using it would essentially mean that we can't ship `split_view` in LLVM16, but I think QoI is more important than having `split_view` now.
I agree we should try to use `pointer_int_pair` here. One thing we could do is ship `split_view` in LLVM 16, but ship (just that view) as experimental, still. We could guard just that view with `_LIBCPP_ENABLE_EXPERIMENTAL`.

Edit: Thinking about it some more, I think we'll always need to `reinterpret_cast` stuff around to make this work. It's not clear to me that it'll ever work inside `constexpr` (quite unfortunately). If we don't think we have a reasonable path forward to make it work inside constexpr, it probably doesn't make sense to artificially delay this "just because". @philnik , unless you can think of a way to make `pointer_int_pair` `constexpr` in the future and you think it is likely to materialize, I wouldn't delay this.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/begin.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
High-level comment: @var-const had found some really surprising behavior when using `lazy_split` with e.g. C-style strings, because the separator would be e.g. `abc\0`. He had a test file where he tested a bunch of these corner cases, which are (unfortunately) mandated by the standard, despite being really surprising for users. I think `split` probably has the same corner cases, so we should also test those. It's probably easy to copy the tests over.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142063