[libcxx-commits] [PATCH] D106507: [libcxx][ranges] Add ranges::take_view.

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 5 inline comments as done.
zoecarver added a subscriber: tcanens.
zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/take_view.h:43
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr take_view(_View __base, range_difference_t<_View> __count)
+      : __base_(_VSTD::move(__base)), __count_(__count) {}
----------------
cjdb wrote:
> Can be `const`.
`__count`? I don't really see how that benefits us... it's passed by value...


================
Comment at: libcxx/include/__ranges/take_view.h:53
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr auto begin() requires (!__simple_view<_View>) {
+      if constexpr (sized_range<_View>) {
----------------
cjdb wrote:
> zoecarver wrote:
> > Chris, before you ask, I did try to factor this out into a static member function, but ran into a couple of issues (first, I had to inline `size`, second, it didn't work well for move only views). 
> > 
> > 😉 
> Now that you've brought it to my attention... ;)
> 
> * why did you need to inline `size`?
> * why doesn't it work well for move-only views?
Well, because we really want a static function that we pass `__base_` to, otherwise that kind of defeats the purpose of deducing `_View` vs `const _View`. 

If we're passing `__base_` we both lose `size` and have to move it into the function and then out of the function when we put it back into the member. That seems like an unreasonable cost. 


================
Comment at: libcxx/include/__ranges/take_view.h:110
+      auto __n = ranges::size(__base_);
+      return _VSTD::min(__n, static_cast<decltype(__n)>(__count_));
+    }
----------------
ldionne wrote:
> cjdb wrote:
> > Hmm... I just realised that if we're going to be using algorithms here, we should definitely be using ranges overloads. You won't be stepping on my turf if you implement the min/max family.
> It seems conforming to me to use `std::min` here, no?
I think that's a bit out of scope for my current timeline. Is there an observable difference? 


================
Comment at: libcxx/include/__ranges/take_view.h:135
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr explicit __sentinel(sentinel_t<_Base> __end) : __end_(__end) {} // TODO: why not move this one too?
+
----------------
ldionne wrote:
> cjdb wrote:
> > zoecarver wrote:
> > > We move `__end` in the other constructor, why not here too? We don't use it later. 
> > If it's passed by value we should be allowed to do so.
> I say we should move it. I don't see a reason either.
Why did the standard do it below and not here, though? Was that just an oversight? CC @tcanens 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106507