[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 accepted this revision as: var-const.
var-const added a comment.

Addressed comments look good, I'll hold off final approval until the test coverage is finished. Thanks!



================
Comment at: libcxx/include/__ranges/split_view.h:120
+private:
+  bool __trailing_empty_                                        = false;
+  split_view<_View, _Pattern>* __parent_                        = nullptr;
----------------
ldionne wrote:
> 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.
We don't do this optimization in a very similar case of `lazy_split_view::outer-iterator` (neither do the other implementations, for `lazy_split_view` or `split_view`). More importantly, is this optimization worth doing? It adds a runtime penalty on many invocations of `operator++` and `operator==`, while I'm not sure if the benefit is worth it -- it makes the iterator type occupy less space, but it still won't fit into a register, and it doesn't seem likely that users are going to store it often. It seems like it would penalize the most common use case for the sake of a more niche one.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142063