[libcxx-commits] [PATCH] D141699: [libc++][ranges] Implement P2474R2(`views::repeat`).

Mon Mar 27 02:28:21 PDT 2023

philnik added inline comments.

================
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|"
----------------
Please add a release note.

================
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))) {}
----------------
yronglin wrote:
> philnik wrote:
> > 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))`.
> I have tried to add new ctor to __copyable_box, but is_constructible_v<_Tp, _Args...> still need a workaround, so, can we add this new ctor and remove `__repeat_view_constructible_from` in LLVM 18? WDYH?
This shouldn't be a problem anymore, since the workaround is removed, right?

================
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))) {}
+
----------------
yronglin wrote:
> yronglin wrote:
> > philnik wrote:
> > > yronglin wrote:
> > > > philnik wrote:
> > > > > Why not simply use `std::make_from_tuple`?
> > > > `__repeat_view_make_from_tuple` still is a workaround for older version clang (https://godbolt.org/z/dreT87G4f). as far as I know, `std::make_from_tuple` initializing aggregates from a parenthesized list of values( https://github.com/llvm/llvm-project/blob/c0b4ca107a3b605f810bd60642907e6a77f7c6d3/libcxx/include/tuple#L1835 ), but this code requires P0960
> > > Looking at your example, this doesn't seem like something that's specific to `repeat_view`. I'd just remove the workarounds and XFAIL the tests that don't work. The extra amount of code to make this work really isn't worth the LLVM15 support, which almost nobody uses anyways.
> > Thanks, I will try to remove this workaround.
> @philnik could you please help me, is it correct to use `// XFAIL: clang-14, clang-15, apple-clang-14` disable this test on old version clang? the CI seems run this test with clang-15/14
The XFAIL looks correct. The CI is currently failing earlier, so I can't see what the problem is exactly. Could you ping me again if you run into this again?

================
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>> &&
----------------
yronglin wrote:
> philnik wrote:
> > Please add a regression test!
> > Please add a regression test!
> 
> I think here is `copy_constructible`, not `move_constructible` ?
> ```
> template<copy_constructible W, semiregular Bound = unreachable_sentinel_t>
>     requires is-integer-like<Bound> || same_as<Bound, unreachable_sentinel_t>
>   class repeat_view<W, Bound>::iterator {
>   private:
>   ......
>   };
> ```
> 
> https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2474r2.html#wording-range.repeat.iterator
The working draft uses `move_constructible`: http://eel.is/c++draft/range.repeat#iterator. Could you look into when this changed and apply the changes to your patch?

================
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:
> Let's just forward-delcare `repeat_view` instead.
It seems you haven't don this?

================
Comment at: libcxx/test/libcxx/ranges/range.factories/range.repeat.view/ctor.value.bound.verify.cpp:20
+int main(int, char**) {
+  { TEST_LIBCPP_ASSERT_FAILURE(std::ranges::repeat_view(0, -1), "The value of bound must be greater equal than 0"); }
+
----------------
Why do you have an extra scope around this?

================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp:29
+
+int main(int, char**) { return 0; }
----------------
You don't need a `main` in a `.compile.pass.cpp` test.

================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp:17
+#include <iterator>
+
+struct Empty {};
----------------
philnik wrote:
> Please add tests for the `explicit`s and constraints.
You seem to be missing tests for the `copy_constructible` constraint.

================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/increment.pass.cpp:32-33
+
+  static_assert(!std::is_reference_v<decltype(iter2++)>);
+  static_assert(std::is_reference_v<decltype(++iter2)>);
+
----------------
This seems redundant, since you already check for the specific type below.

================
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
----------------
philnik wrote:
> Let's just check `sizeof(Iter::difference_type)) == sizeof(unsigned)` to be more platform-agnostic.
This seems to not be done.

================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:569
+    "headers": ["ranges", "tuple", "utility"],
+    "unimplemented": True,
   }, {
----------------
Is something missing from the Paper? You claim it's complete in the status paper.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141699