[libcxx-commits] [PATCH] D141699: [libc++][ranges] Implement P2474R2(`views::repeat`).
Yurong via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Jun 5 10:00:15 PDT 2023
yronglin added a comment.
Sorry for the late reply and thanks a lot for your review! @var-const
================
Comment at: libcxx/docs/FeatureTestMacroTable.rst:347
------------------------------------------------- -----------------
+ ``__cpp_lib_ranges_repeat`` *unimplemented*
+ ------------------------------------------------- -----------------
----------------
var-const wrote:
> Shouldn't this be annotated with the corresponding date rather than being marked `*unimplemented*`?
This value is incorrect.
================
Comment at: libcxx/docs/Status/Cxx2bPapers.csv:80
"`P2467R1 <https://wg21.link/P2467R1>`__","LWG","Support exclusive mode for ``fstreams``","July 2022","",""
-"`P2474R2 <https://wg21.link/P2474R2>`__","LWG","``views::repeat``","July 2022","","","|ranges|"
+"`P2474R2 <https://wg21.link/P2474R2>`__","LWG","``views::repeat``","July 2022","|Complete|","17.0","|ranges|"
"`P2494R2 <https://wg21.link/P2494R2>`__","LWG","Relaxing range adaptors to allow for move only types","July 2022","","","|ranges|"
----------------
philnik wrote:
> Please add a release note.
done.
================
Comment at: libcxx/include/__ranges/drop_view.h:278
+ class _Dist = range_difference_t<_Range>>
+ requires (!__is_empty_view<_RawRange> &&
+ __is_repeat_specialization<_RawRange> &&
----------------
var-const wrote:
> Question: is `__is_empty_view` necessary here?
We don't need `__is_empty_view ` here, I'll remove that.
================
Comment at: libcxx/include/__ranges/drop_view.h:282
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range, _Np&& __n) const noexcept(
+ noexcept(repeat_view(*ranges::begin(__range),
+ ranges::distance(__range) - std::min<_Dist>(ranges::distance(__range), std::forward<_Np>(__n)))))
----------------
var-const wrote:
> Nit: in existing files, we try to format the three expressions (the actual return expression and its two duplicates inside the `noexcept` and the `decltype`) so that all the three are aligned, making it easier to quickly check that they are exactly the same. Can you format these similarly? Feel free to disable clang-format for those lines if it gets in the way.
Thanks for your tips, add `// clang format off` here.
================
Comment at: libcxx/include/__ranges/drop_view.h:284
+ ranges::distance(__range) - std::min<_Dist>(ranges::distance(__range), std::forward<_Np>(__n)))))
+ -> decltype(repeat_view(*ranges::begin(__range),
+ ranges::distance(__range) -
----------------
var-const wrote:
> var-const wrote:
> > In the standard, it uses `views::repeat` rather than `repeat_view`, is there a reason not to do the same here?
> Question: the standard instead uses `__range.value_`, is using `begin` instead deliberate?
I used `ranges::begin` previously, because `__range.__value_` was a private member of `repeat_view`. Nowadays, I make the `std::views::__take::__fn` and `std::views::__drop::__fn` as the friend of `repeat_view`, so we can access the private member `__range.__value_`. WDYT?
================
Comment at: libcxx/include/__ranges/drop_view.h:284
+ ranges::distance(__range) - std::min<_Dist>(ranges::distance(__range), std::forward<_Np>(__n)))))
+ -> decltype(repeat_view(*ranges::begin(__range),
+ ranges::distance(__range) -
----------------
yronglin wrote:
> var-const wrote:
> > var-const wrote:
> > > In the standard, it uses `views::repeat` rather than `repeat_view`, is there a reason not to do the same here?
> > Question: the standard instead uses `__range.value_`, is using `begin` instead deliberate?
> I used `ranges::begin` previously, because `__range.__value_` was a private member of `repeat_view`. Nowadays, I make the `std::views::__take::__fn` and `std::views::__drop::__fn` as the friend of `repeat_view`, so we can access the private member `__range.__value_`. WDYT?
done, use `views::repeat`.
================
Comment at: libcxx/include/__ranges/repeat_view.h:43
+
+template <copy_constructible _Tp, semiregular _Bound = unreachable_sentinel_t>
+ requires(is_object_v<_Tp> && same_as<_Tp, remove_cv_t<_Tp>> &&
----------------
var-const wrote:
> Hmm, it's `move_constructible` in the latest draft of the Standard. Is landing https://reviews.llvm.org/D151629 a prerequisite to do that?
Yes!
================
Comment at: libcxx/include/__ranges/repeat_view.h:45
+ requires(is_object_v<_Tp> && same_as<_Tp, remove_cv_t<_Tp>> &&
+ (__integer_like<_Bound> || same_as<_Bound, unreachable_sentinel_t>))
+class repeat_view : public view_interface<repeat_view<_Tp, _Bound>> {
----------------
var-const wrote:
> var-const wrote:
> > The latest draft of the Standard uses `integer-like-with-usable-difference-type` which seems to have a couple more conditions. If this is deliberate, can you please add a comment with a TODO?
> Question: can `_Bound` be a user-defined type?
LWG3875(https://cplusplus.github.io/LWG/issue3875) introduced `integer-like-with-usable-difference-type` , I think this patch need update to apply this change.
================
Comment at: libcxx/include/__ranges/repeat_view.h:45
+ requires(is_object_v<_Tp> && same_as<_Tp, remove_cv_t<_Tp>> &&
+ (__integer_like<_Bound> || same_as<_Bound, unreachable_sentinel_t>))
+class repeat_view : public view_interface<repeat_view<_Tp, _Bound>> {
----------------
yronglin wrote:
> var-const wrote:
> > var-const wrote:
> > > The latest draft of the Standard uses `integer-like-with-usable-difference-type` which seems to have a couple more conditions. If this is deliberate, can you please add a comment with a TODO?
> > Question: can `_Bound` be a user-defined type?
> LWG3875(https://cplusplus.github.io/LWG/issue3875) introduced `integer-like-with-usable-difference-type` , I think this patch need update to apply this change.
> Question: can `_Bound` be a user-defined type?
As far as I know, `_Bound` can not be a user-define type.
================
Comment at: libcxx/include/__ranges/repeat_view.h:54
+
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit repeat_view(const _Tp& __value, _Bound __bound_sentinel = _Bound())
+ : __value_(in_place, __value), __bound_(__bound_sentinel) {
----------------
var-const wrote:
> `requires copy_constructible<T>`?
The constraint `requires copy_constructible<T>` introduced by https://open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2494r2.html#wording-range.adjacent.transform.view . Once D151629 landed, I'll add `requires copy_constructible<T>`.
================
Comment at: libcxx/include/__ranges/repeat_view.h:68
+ requires(constructible_from<_Tp, _TpArgs...> && constructible_from<_Bound, _BoundArgs...>)
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit repeat_view(
+ piecewise_construct_t, tuple<_TpArgs...> __value_args, tuple<_BoundArgs...> __bound_args = tuple<>{})
----------------
var-const wrote:
> Perhaps we could add an assertion for the `The behavior is undefined if Bound is not unreachable_sentinel_t and bound_ is negative` clause?
done!
================
Comment at: libcxx/include/__ranges/repeat_view.h:109
+ if constexpr (!same_as<_Bound, unreachable_sentinel_t>)
+ _LIBCPP_ASSERT(__current_ >= 0, "The value of bound must be greater equal than 0");
+ }
----------------
var-const wrote:
> Question: this is already asserted in the constructor of the view. What is the situation where it needs to be asserted again when creating the iterator?
Good catch! I think it's unnecessary, I'll remove it.
================
Comment at: libcxx/include/__ranges/take_view.h:34
#include <__ranges/range_adaptor.h>
+#include <__ranges/repeat_view.h>
#include <__ranges/size.h>
----------------
philnik wrote:
> philnik wrote:
> > Let's just forward-delcare `repeat_view` instead.
> It seems you haven't don this?
Sorry, somehow I have missed this, I've added `__fwd/repeat_view.h` now.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp:21
+// clang-format off
+static_assert(std::same_as<decltype(std::ranges::repeat_view(Empty())), std::ranges::repeat_view<Empty> >);
+static_assert(std::same_as<decltype(std::ranges::repeat_view(std::declval<Empty&>())), std::ranges::repeat_view<Empty> >);
----------------
var-const wrote:
> Ultranit: if the space between the two closing angle brackets is deliberate, then perhaps also add a space after the corresponding opening angle bracket?
Thanks, I'll remove this whitespace
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp:29
+
+int main(int, char**) { return 0; }
----------------
var-const wrote:
> philnik wrote:
> > You don't need a `main` in a `.compile.pass.cpp` test.
> Nit: this `main` is unnecessary.
Thanks, removed.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/end.pass.cpp:30
+ {
+ std::ranges::repeat_view<int> rv(0);
+ assert(std::ranges::next(rv.begin(), 10) != rv.end());
----------------
var-const wrote:
> Question: do we have a test where we iterate over a whole bounded view, i.e. go from `begin` to `end` and make sure the same value is repeated?
Good catch, I'll try to add a new test case.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/compare.pass.cpp:32
+
+ assert((iter1 <=> iter2) == std::strong_ordering::less);
+ assert((iter1 <=> iter1) == std::strong_ordering::equal);
----------------
var-const wrote:
> I think we should also check the generated comparison operators (unless there's precedent for not doing so in existing tests).
Thanks, I'll add test to check the generated comparison operators.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/ctor.default.pass.cpp:17
+ using Iter = std::ranges::iterator_t<std::ranges::repeat_view<int>>;
+ static_assert(std::is_nothrow_default_constructible_v<Iter>);
+ Iter iter;
----------------
var-const wrote:
> Question: is the default constructor guaranteed to be `nothrow`?
Nope, I have not found any words that guaranteed the default ctor to be `nothrow` in the latest draft, I'll try to use `std::is_default_constructible_v`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/ctor.default.pass.cpp:19
+ Iter iter;
+ (void)iter;
+
----------------
var-const wrote:
> var-const wrote:
> > Nit: in new code, we prefer to use `[[maybe_unused]]` instead of a void cast.
> Are there any properties of a default-constructed iterator that we could check? It it perhaps equal to `end()`?
IMO, if we want to check the properties of a default-constructed iterator, we can only check the `__current_ = _IndexT()`
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/decrement.pass.cpp:22
+
+ assert(iter1 == iter2);
+ assert(iter1-- == iter2--);
----------------
var-const wrote:
> Rather than comparing the two iterators, consider comparing the return values to `rv.begin() + some-index`, i.e. something like `assert(iter1-- == rv.begin() + 9);`.
agree
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/decrement.pass.cpp:30
+
+ static_assert(!std::is_reference_v<decltype(iter2++)>);
+ static_assert(std::is_reference_v<decltype(++iter2)>);
----------------
var-const wrote:
> Can you check the exact type? That would be more specific and replace all the 3 `static_assert`s.
agree
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp:53
+
+ // If we're compiling for 32 bit or windows, int and long are the same size, so long long is the correct difference type.
+#if INTPTR_MAX == INT32_MAX || defined(_WIN32)
----------------
var-const wrote:
> Would it be possible to use `std::uint64_t` or similar?
done
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus.pass.cpp:26
+ assert(iter1 == iter2);
+ assert(iter1 - 5 != iter2);
+ assert(iter1 - 5 == std::ranges::prev(iter2, 5));
----------------
var-const wrote:
> Nit: I don't think this check adds much value.
Thanks, removed.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/subscript.pass.cpp:21
+ for (int i = 0; i < 100; ++i)
+ assert(iter[i] == 31);
+
----------------
var-const wrote:
> Nit: use `std::ranges::all_of`?
done!
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp:11
+
+// views::repeat
+
----------------
var-const wrote:
> Nit: it would be helpful to have the constraints here as well.
done.
================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:569
+ "headers": ["ranges", "tuple", "utility"],
+ "unimplemented": True,
}, {
----------------
philnik wrote:
> Is something missing from the Paper? You claim it's complete in the status paper.
Oops, this value should be False.
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