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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb requested changes to this revision.
cjdb added a comment.
This revision now requires changes to proceed.

Please add range concept conformance tests before merging.



================
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) {}
----------------
zoecarver wrote:
> cjdb wrote:
> > Can be `const`.
> `__count`? I don't really see how that benefits us... it's passed by value...
If we start by making everything const and then remove it for only the objects we intend to modify, we're better-documenting our intention. Put another way: we shouldn't need a reason to provide `const`: we should have a good reason to //omit// `const`.


================
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>) {
----------------
ldionne wrote:
> zoecarver wrote:
> > 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. 
> I think this would work:
> 
> ```
> template <class _Self>
> _LIBCPP_HIDE_FROM_ABI
> static constexpr auto __begin(_Self& __self) {
>   using _MaybeConstView = _If<is_const_v<_Self>, _View const, _View>;
>   if constexpr (sized_range< _MaybeConstView >) {
>         if constexpr (random_access_range<_MaybeConstView>) {
>           return ranges::begin(__self.__base_);
>         } else {
>           using _DifferenceT = range_difference_t<_MaybeConstView>;
>           auto __size = __self.size();
>           return counted_iterator{ranges::begin(__self.__base_), static_cast<_DifferenceT>(__size)};
>         }
>       } else {
>         return counted_iterator{ranges::begin(__self.__base_), __self.__count_};
>       }
> }
> 
> _LIBCPP_HIDE_FROM_ABI
> constexpr auto begin() requires (!__simple_view<_View>) {
>   return take_view::__begin(*this);
> }
> 
> // etc...
> ```
> 
> I'm still not sure whether I like that better than matching the spec 1:1. I'll leave it to you to decide @zoecarver .
@ldionne's got what I had in mind. I like it because it keeps the complex logic in one place and makes sure that any bugs only have one source of manifestation.


================
Comment at: libcxx/include/__ranges/take_view.h:110
+      auto __n = ranges::size(__base_);
+      return _VSTD::min(__n, static_cast<decltype(__n)>(__count_));
+    }
----------------
zoecarver wrote:
> 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? 
In general: yes, there is an observable difference. `std::min` requires that its parameters meet //Cpp17LessThanComparable//, whereas `std::ranges::min` requires that its parameters model `totally_ordered_with`. Since we're comparing integers, I don't think it's going to be an observable difference.

Could you please at least add a FIXME so I remember to clean it up one day?


================
Comment at: libcxx/test/support/test_iterators.h:701
 
-  cpp20_input_iterator() = default;
+  cpp20_input_iterator() = delete;
 
----------------
Why is this being deleted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106507