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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


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


================
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:
> var-const wrote:
> > 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?
> I think so, since it's an extension. I think we used `[[nodiscard]]` before because we wanted the extension for most users, but back then we didn't enable the `[[nodiscard]]` extensions by default. Now we do, so we can just say "don't disable the extension if you want it".
> 
> Looking through the views, it seems like we use `[[nodiscard]]` on views, and didn't declare `views::iota`, `views::istream` and `views::zip` with any `[[nodiscard]]` version.
let's do the others in a separate change


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.split/begin.pass.cpp:151
+    auto calledTimesAfterFirstBegin = equalsCalledTimes;
+    assert(calledTimesAfterFirstBegin != 0);
+
----------------
var-const wrote:
> Question: is it not guaranteed to be `1`?
I am not totally sure as that is the implementation detail of `std::ranges::search`. What we care here is that the first time it is called the numbers go up and from the second time, the numbers won't go up anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142063