[libcxx-commits] [PATCH] D106507: [libcxx][ranges] Add ranges::take_view.
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Jul 26 11:21:28 PDT 2021
ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.
LGTM mod comments and passing CI!
================
Comment at: libcxx/include/__ranges/take_view.h:76
+ auto __size = size();
+ return counted_iterator{ranges::begin(__base_), static_cast<_DifferenceT>(__size)};
+ }
----------------
In all those `counted_iterator` constructions, the spec uses direct-initialization instead of list-initialization. I don't think this ever matters because there can't be a narrowing conversion from `_DifferenceT` to the argument of `counted_iterator::counted_iterator` (both types should always be exactly `iter_difference_t<iterator_t<_View>>`), but we might as well follow the spec.
================
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>) {
----------------
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 .
================
Comment at: libcxx/include/__ranges/take_view.h:154
+ friend constexpr bool operator==(const _Iter<_Const>& __lhs, const __sentinel& __rhs) {
+ return __lhs.count() == 0 || __lhs.base() == __rhs.base();
+ }
----------------
zoecarver wrote:
> ldionne wrote:
> > You could use `__rhs.__end_` here, which would avoid making a copy of the sentinel (`base()` makes a copy). Same applies above.
> That really //should// be inlined. But fair enough. I'm going to make it public so I can do that for lhs too.
Depending on what the `_View`'s copy constructor does, it might not be possible to inline it, I think. I would definitely not rely on that, at least.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/base.pass.cpp:23
+
+struct ContiguousView : std::ranges::view_base {
+ int *ptr_;
----------------
Can you make this more minimal? For example, I think you could provide only `begin() const` as a member function.
Also, since you're using it in a bunch of places, perhaps you could put it in a `types.h` helper header in this directory? That would be a simple way to remove some duplication.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/begin.pass.cpp:96
+
+ {
+ std::ranges::take_view<SizedRandomAccessView> tv(SizedRandomAccessView{buffer}, 4);
----------------
Could you add a one-liner to each test to make it very obvious which branch of the `if constexpr` you're testing? Something as simple as:
```
// sized_range && random_access
```
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/begin.pass.cpp:132-142
+ {
+ std::ranges::take_view<CopyableView> tv(CopyableView{buffer}, 4);
+ assert(tv.begin() == std::counted_iterator<int*>(buffer, 4));
+ ASSERT_SAME_TYPE(decltype(tv.begin()), std::counted_iterator<int*>);
+ }
+
+ {
----------------
I'm not sure what coverage this is adding. Consider removing if it doesn't add anything.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/ctad.compile.pass.cpp:17
+#include <ranges>
+#include <cassert>
+
----------------
`#include <concepts>`
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/ctad.compile.pass.cpp:45
+
+void testCTAD() {
+ View v;
----------------
Alternatively, you could leave at file scope. Whatever you prefer.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/ctor.pass.cpp:63
+ {
+ static_assert( std::default_initializable<DefaultConstructible<true>>);
+ static_assert(!std::default_initializable<DefaultConstructible<false>>);
----------------
You never actually default-construct an object here. Could you do that? Ideally, you'd then test that `base()` has been default-initialized, and that the `size()` is `0` (which you could do best by creating a view that has non-zero size when default-constructed).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/end.pass.cpp:96
+
+ {
+ std::ranges::take_view<SizedRandomAccessView> tv(SizedRandomAccessView{buffer}, 0);
----------------
Same comment as for `begin()` w.r.t. which `if` branch we're testing.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/size.pass.cpp:112
+ {
+ std::ranges::take_view<SizedForwardView> tv(SizedForwardView{buffer}, 8);
+ assert(tv.size() == 8);
----------------
You want to test this version (N == size-of-the-range) for const too.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/size.pass.cpp:117
+ {
+ const std::ranges::take_view<SizedForwardView> tv(SizedForwardView{buffer}, 10);
+ assert(tv.size() == 8);
----------------
You want to test this version (N > size-of-the-range) for non-const too.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106507/new/
https://reviews.llvm.org/D106507
More information about the libcxx-commits
mailing list