[libcxx-commits] [PATCH] D141699: [libc++][ranges] Implement P2474R2(`views::repeat`).
Nikolas Klauser via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Mar 3 09:00:26 PST 2023
philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.
In D141699#4166980 <https://reviews.llvm.org/D141699#4166980>, @yronglin wrote:
> ping
Sorry that it takes so long. The `view` patches are always really large, so it takes quite a bit of time to get through it all. The implementation looks mostly good, but the tests need some work.
================
Comment at: libcxx/include/__ranges/drop_view.h:284
+ sized_range<_RawRange>)
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range, _Np&& __n) const noexcept(
+ noexcept(views::repeat(*ranges::begin(__range),
----------------
================
Comment at: libcxx/include/__ranges/repeat_view.h:30
+#include <tuple>
+#include <type_traits>
+
----------------
Please granularize the include.
================
Comment at: libcxx/include/__ranges/repeat_view.h:52
+
+// std::ranges::repeat_view requires P0960, but P0960 supported since clang 16.
+// This workaround used to enable repeat_view on the older versions clang.
----------------
Could you add a `// TODO LLVM18: remove this workaround` to make it easier to find?
================
Comment at: libcxx/include/__ranges/repeat_view.h:100
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit repeat_view(_Tp&& __value, _Bound __bound_sentinel = _Bound())
+ : __value_(std::in_place, std::move(__value)), __bound_(__bound_sentinel) {
+ if constexpr (!same_as<_Bound, unreachable_sentinel_t>)
----------------
================
Comment at: libcxx/include/__ranges/repeat_view.h:106-107
+ template <class... _TpArgs, class... _BoundArgs>
+ requires(std::ranges::__repeat_view_constructible_from<_Tp, _TpArgs...> &&
+ std::ranges::__repeat_view_constructible_from<_Bound, _BoundArgs...>)
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit repeat_view(
----------------
These aren't subject to ADL, so we don't have to qualify them.
================
Comment at: libcxx/include/__ranges/repeat_view.h:110
+ piecewise_construct_t, tuple<_TpArgs...> __value_args, tuple<_BoundArgs...> __bound_args = tuple<>{})
+ : __value_(std::in_place, std::ranges::__repeat_view_make_from_tuple<_Tp>(std::move(__value_args))),
+ __bound_(std::ranges::__repeat_view_make_from_tuple<_Bound>(std::move(__bound_args))) {}
----------------
I think we should instead add new constructors to `__copyable_box`, so we can construct the type in-place. For example (not tested):
```lang=c++
template <class... _Args, size_t... _Indices>
requires is_constructible_v<_Tp, _Args...>
_LIBCPP_HIDE_FROM_ABI constexpr explicit __copyable_box(
piecewise_construct_t, tuple<_Args...> __tuple, index_sequence<_Indices...>)
: __copyable_box(in_place, std::get<_Indices>(std::forward<tuple<_Args...>>(__tuple))...) {}
template <class... _Args>
requires is_constructible_v<_Tp, _Args...>
_LIBCPP_HIDE_FROM_ABI constexpr explicit __copyable_box(piecewise_construct_t, tuple<_Args...> __args)
: __copyable_box(std::move(__args), std::make_index_sequence<sizeof...(_Args)>()) {}
```
Then we can simply do `__value_(piecewise_construct, std::move(__value_args))`.
================
Comment at: libcxx/include/__ranges/repeat_view.h:111
+ : __value_(std::in_place, std::ranges::__repeat_view_make_from_tuple<_Tp>(std::move(__value_args))),
+ __bound_(std::ranges::__repeat_view_make_from_tuple<_Bound>(std::move(__bound_args))) {}
+
----------------
Why not simply use `std::make_from_tuple`?
================
Comment at: libcxx/include/__ranges/repeat_view.h:130
+private:
+ __copyable_box<_Tp> __value_ = __copyable_box<_Tp>(std::in_place, _Tp());
+ _LIBCPP_NO_UNIQUE_ADDRESS _Bound __bound_ = _Bound();
----------------
================
Comment at: libcxx/include/__ranges/repeat_view.h:138
+// [range.repeat.iterator]
+template <copy_constructible _Tp, semiregular _Bound>
+ requires(is_object_v<_Tp> && same_as<_Tp, remove_cv_t<_Tp>> &&
----------------
Please add a regression test!
================
Comment at: libcxx/include/__ranges/repeat_view.h:144
+
+ using _IndexT = conditional_t<same_as<_Bound, unreachable_sentinel_t>, ptrdiff_t, _Bound >;
+
----------------
================
Comment at: libcxx/include/__ranges/repeat_view.h:146
+
+ constexpr explicit __iterator(const _Tp* __value, _IndexT __bound_sentinel = _IndexT())
+ : __value_(__value), __current_(__bound_sentinel) {
----------------
================
Comment at: libcxx/include/__ranges/repeat_view.h:156
+ using value_type = _Tp;
+ using difference_type = conditional_t<__signed_integer_like<_IndexT>, _IndexT, _IotaDiffT<_IndexT> >;
+
----------------
================
Comment at: libcxx/include/__ranges/repeat_view.h:235-239
+template <class _Tp>
+concept __can_unbound_repeat_view = requires { repeat_view(std::declval<_Tp>()); };
+
+template <class _Tp, class _Bound>
+concept __can_bound_repeat_view = requires { repeat_view(std::declval<_Tp>(), std::declval<_Bound>()); };
----------------
IMO we should just inline the `requires` clauses.
================
Comment at: libcxx/include/__ranges/repeat_view.h:267
+
+#endif // _LIBCPP_STD_VER > 17
+
----------------
================
Comment at: libcxx/include/__ranges/take_view.h:34
#include <__ranges/range_adaptor.h>
+#include <__ranges/repeat_view.h>
#include <__ranges/size.h>
----------------
Let's just forward-delcare `repeat_view` instead.
================
Comment at: libcxx/include/__ranges/take_view.h:220-226
+#if _LIBCPP_STD_VER >= 23
+template <class _Tp>
+inline constexpr bool __is_repeat_specialization = false;
+
+template <class _Tp, class _Bound>
+inline constexpr bool __is_repeat_specialization<repeat_view<_Tp, _Bound>> = true;
+#endif
----------------
Let's move this to `__fwd/repeat_view.h`, so we don't define this multiple times in the code base.
================
Comment at: libcxx/include/__ranges/take_view.h:317
+ requires(!__is_empty_view<_RawRange> && __is_repeat_specialization<_RawRange> && sized_range<_RawRange>)
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range, _Np&& __n) const noexcept(noexcept(
+ views::repeat(*ranges::begin(__range), std::min<_Dist>(ranges::distance(__range), std::forward<_Np>(__n)))))
----------------
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/begin.pass.cpp:10-11
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+
----------------
Both of these are already removed from the code base. Please make sure they are removed from all the new tests.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/begin.pass.cpp:20
+constexpr bool test() {
+ {
+ std::ranges::repeat_view<int> rv(0);
----------------
Let's also add tests when we don't have `unreachable_sentinel`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp:21
+constexpr bool test() {
+ static_assert(std::same_as< decltype(std::ranges::repeat_view(Empty())), std::ranges::repeat_view<Empty> >);
+
----------------
Same below.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp:39-43
+int main(int, char**) {
+ test();
+ static_assert(test());
+ return 0;
+}
----------------
You can just put the `static_assert`s above at the global scope and remove this boilerplate.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.default.pass.cpp:16
+#include <concepts>
+
+struct DefaultInt42 {
----------------
Please add a negative test for the constraint.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.default.pass.cpp:22
+constexpr bool test() {
+ std::ranges::repeat_view<DefaultInt42> rv(DefaultInt42{});
+ assert((*rv.begin()).__value_ == 42);
----------------
This doesn't test the default constructor.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.piecewise.pass.cpp:21
+#include <utility>
+
+struct A {
----------------
Please also add tests for the constraints.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.piecewise.pass.cpp:22-25
+struct A {
+ int x_;
+ int y_;
+};
----------------
Let's add a constructor that only takes very specific arguments to make sure we actually `std::forward` the types.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.piecewise.pass.cpp:33
+ assert(std::ranges::next(rv.begin(), 3) == rv.end());
+
+ return true;
----------------
Let's also test that you can explicitly construct the `unrechable_sentinel` from a `tuple{}`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp:17
+#include <iterator>
+
+struct Empty {};
----------------
Please add tests for the `explicit`s and constraints.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp:19
+struct Empty {};
+
+constexpr bool test() {
----------------
Please also add tests to make sure the `_LIBCPP_ASSERT`s actually work in `libcxx/test/libcxx`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp:47
+ {
+ Empty e;
+ std::ranges::repeat_view<Empty> rv(e);
----------------
To make sure it doesn't just take mutable references.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/end.pass.cpp:18
+#include <utility>
+
+constexpr bool test() {
----------------
Let's also add a test to make sure `end()` is `noexcept` for an `unreachable_sentinel`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/compare.pass.cpp:22
+
+ std::same_as<R> auto r = std::views::repeat(42);
+ auto iter1 = r.begin();
----------------
Let's not use CTAD here.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/compare.pass.cpp:28
+ assert(!(iter1 == iter2));
+ assert(iter2 == iter2);
+
----------------
Let's also check the return types.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/ctor.default.pass.cpp:14
+#include <ranges>
+
+constexpr bool test() {
----------------
Please also make sure that `repeat_view::iterator` `is_nothrow_default_constructible`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/increment.pass.cpp:30-33
+ static_assert(!std::is_reference_v<decltype(iter2++)>);
+ static_assert(std::is_reference_v<decltype(++iter2)>);
+
+ static_assert(std::same_as<std::remove_reference_t<decltype(++iter2)>, decltype(iter2++)>);
----------------
Let's just test for the specific type instead.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/increment.pass.cpp:33
+
+ static_assert(std::same_as<std::remove_reference_t<decltype(++iter2)>, decltype(iter2++)>);
+
----------------
Missing `<concepts>` include
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp:23
+#include <ranges>
+
+constexpr bool test() {
----------------
Let's also test with a user-defined type (especially a move-only type).
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp:27-28
+ {
+ const std::ranges::repeat_view<int> v(0);
+ using Iter = decltype(v.begin());
+ static_assert(std::same_as<Iter::iterator_concept, std::random_access_iterator_tag>);
----------------
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp:33
+ static_assert(std::same_as<Iter::difference_type, ptrdiff_t>);
+ static_assert(std::is_signed_v<Iter::difference_type>);
+ }
----------------
This doesn't hurt, but also doesn't make a lot of sense if we test for a specific type.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp:57-61
+#if INTPTR_MAX == INT32_MAX || defined(_WIN32)
+ static_assert(std::same_as<Iter::difference_type, long long>);
+#else
+ static_assert(std::same_as<Iter::difference_type, long>);
+#endif
----------------
Let's just check `sizeof(Iter::difference_type)) == sizeof(unsigned)` to be more platform-agnostic.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/plus.pass.cpp:24
+ assert(iter1 + 5 != iter2);
+ assert(iter1 + 5 == std::ranges::next(iter2, 5));
+ assert(5 + iter1 != iter2);
----------------
This makes it a bit easier to verify at a glance.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/plus.pass.cpp:30-31
+
+ static_assert(!std::is_reference_v<decltype(iter2 + 5)>);
+ static_assert(!std::is_reference_v<decltype(5 + iter2)>);
+
----------------
Let's check the exact type instead.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/plus_eq.pass.cpp:26
+
+ static_assert(std::is_reference_v<decltype(iter2 += 5)>);
+
----------------
Let's check the return type exactly, and make sure it's the same object as `iter2` (i.e. compare the address).
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/star.pass.cpp:21
+ for (int i = 0; i < 100; ++i, ++iter)
+ assert(*iter == 31);
+
----------------
The returned type should always point to the same object IIUC. Let's test that too.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/star.pass.cpp:24
+ static_assert(noexcept(*iter));
+ static_assert(std::is_reference_v<decltype(*iter)>);
+ static_assert(std::same_as<decltype(*iter), const int&>);
----------------
This seems redundant given the `static_assert` below.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp:16
+#include <concepts>
+
+constexpr bool test() {
----------------
Please add negative tests for the constraints of `repeat_view`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141699/new/
https://reviews.llvm.org/D141699
More information about the libcxx-commits
mailing list