[libc++][ranges] Implement P2474R2(`views::repeat`).

[yronglin]

Summary

Implement https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2474r2.html

[philnik]

libcxx/include/__ranges/drop_view.h

281

libcxx/include/__ranges/repeat_view.h

53

Could you add a // TODO LLVM18: remove this workaround to make it easier to find?

101

107–108

These aren't subject to ADL, so we don't have to qualify them.

111

I think we should instead add new constructors to __copyable_box, so we can construct the type in-place. For example (not tested):

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)).

112

Why not simply use std::make_from_tuple?

131

139

Please add a regression test!

145

147

157

236–240

IMO we should just inline the requires clauses.

268

libcxx/include/__ranges/take_view.h

34

Let's just forward-delcare repeat_view instead.

223–229

Let's move this to __fwd/repeat_view.h, so we don't define this multiple times in the code base.

312

libcxx/test/std/ranges/range.factories/range.repeat.view/begin.pass.cpp

11–12

Both of these are already removed from the code base. Please make sure they are removed from all the new tests.

21

Let's also add tests when we don't have unreachable_sentinel.

libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp

22

Same below.

40–44

You can just put the static_asserts above at the global scope and remove this boilerplate.

libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.default.pass.cpp

17

Please add a negative test for the constraint.

23

This doesn't test the default constructor.

libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.piecewise.pass.cpp

22

Please also add tests for the constraints.

23–26

Let's add a constructor that only takes very specific arguments to make sure we actually std::forward the types.

34

Let's also test that you can explicitly construct the unrechable_sentinel from a tuple{}.

libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp

18

Please add tests for the explicits and constraints.

20

Please also add tests to make sure the _LIBCPP_ASSERTs actually work in libcxx/test/libcxx.

48

To make sure it doesn't just take mutable references.

libcxx/test/std/ranges/range.factories/range.repeat.view/end.pass.cpp

19

Let's also add a test to make sure end() is noexcept for an unreachable_sentinel.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/compare.pass.cpp

23

Let's not use CTAD here.

29

Let's also check the return types.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/ctor.default.pass.cpp

15

Please also make sure that repeat_view::iterator is_nothrow_default_constructible.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/increment.pass.cpp

31–34

Let's just test for the specific type instead.

34

Missing <concepts> include

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp

24

Let's also test with a user-defined type (especially a move-only type).

28–29

34

This doesn't hurt, but also doesn't make a lot of sense if we test for a specific type.

58–62

Let's just check sizeof(Iter::difference_type)) == sizeof(unsigned) to be more platform-agnostic.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/plus.pass.cpp

25

This makes it a bit easier to verify at a glance.

31–32

Let's check the exact type instead.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/plus_eq.pass.cpp

27

Let's check the return type exactly, and make sure it's the same object as iter2 (i.e. compare the address).

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/star.pass.cpp

22

The returned type should always point to the same object IIUC. Let's test that too.

25

This seems redundant given the static_assert below.

libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp

17

Please add negative tests for the constraints of repeat_view.

[yronglin]

Thanks a lot for your review @philnik , and sorry for the late reply, I have address most comments, and improve test cases.

libcxx/include/__ranges/repeat_view.h

112

__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

139

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

[yronglin]

Fix CI.

libcxx/include/__ranges/repeat_view.h

111

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?

[yronglin]

Fix CI.

[yronglin]

Fix CI

[philnik]

libcxx/include/__ranges/repeat_view.h

112

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.

[yronglin]

libcxx/include/__ranges/repeat_view.h

112

Thanks, I will try to remove this workaround.

[yronglin]

Remove workaround.

[yronglin]

The transitive_includes test case always fails locally on my laptop (ninja check-cxx)

***************
*** 162,171 ****
--- 162,186 ----
  expectedinitializer_list
  expectednew
  expectedversion
+ experimental/algorithmalgorithm
+ experimental/algorithmcstddef
+ experimental/algorithmtype_traits
+ experimental/coroutinecstddef
+ experimental/coroutinecstdint
+ experimental/coroutinecstring
+ experimental/coroutinelimits
+ experimental/coroutinenew
+ experimental/coroutinetype_traits
  experimental/dequedeque
  experimental/dequeexperimental/memory_resource
  experimental/forward_listexperimental/memory_resource
  experimental/forward_listforward_list
+ experimental/functionalarray
+ experimental/functionalcstddef
+ experimental/functionalfunctional
+ experimental/functionaltype_traits
+ experimental/functionalunordered_map
+ experimental/functionalvector
  experimental/iteratorcstddef
  experimental/iteratoriosfwd
  experimental/iteratoriterator

error: command failed with exit status: 1

my host compiler is

Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: arm64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

[philnik]

In D141699#4169743, @yronglin wrote:

The transitive_includes test case always fails locally on my laptop (ninja check-cxx)

***************
*** 162,171 ****
--- 162,186 ----
  expectedinitializer_list
  expectednew
  expectedversion
+ experimental/algorithmalgorithm
+ experimental/algorithmcstddef
+ experimental/algorithmtype_traits
+ experimental/coroutinecstddef
+ experimental/coroutinecstdint
+ experimental/coroutinecstring
+ experimental/coroutinelimits
+ experimental/coroutinenew
+ experimental/coroutinetype_traits
  experimental/dequedeque
  experimental/dequeexperimental/memory_resource
  experimental/forward_listexperimental/memory_resource
  experimental/forward_listforward_list
+ experimental/functionalarray
+ experimental/functionalcstddef
+ experimental/functionalfunctional
+ experimental/functionaltype_traits
+ experimental/functionalunordered_map
+ experimental/functionalvector
  experimental/iteratorcstddef
  experimental/iteratoriosfwd
  experimental/iteratoriterator

error: command failed with exit status: 1

my host compiler is

Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: arm64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

You probably still have experimental/{algorithm, coroutine, functional} in your local install dir.

[yronglin]

Fix CI

[yronglin]

In D141699#4169749, @philnik wrote:

In D141699#4169743, @yronglin wrote:

The transitive_includes test case always fails locally on my laptop (ninja check-cxx)

***************
*** 162,171 ****
--- 162,186 ----
  expectedinitializer_list
  expectednew
  expectedversion
+ experimental/algorithmalgorithm
+ experimental/algorithmcstddef
+ experimental/algorithmtype_traits
+ experimental/coroutinecstddef
+ experimental/coroutinecstdint
+ experimental/coroutinecstring
+ experimental/coroutinelimits
+ experimental/coroutinenew
+ experimental/coroutinetype_traits
  experimental/dequedeque
  experimental/dequeexperimental/memory_resource
  experimental/forward_listexperimental/memory_resource
  experimental/forward_listforward_list
+ experimental/functionalarray
+ experimental/functionalcstddef
+ experimental/functionalfunctional
+ experimental/functionaltype_traits
+ experimental/functionalunordered_map
+ experimental/functionalvector
  experimental/iteratorcstddef
  experimental/iteratoriosfwd
  experimental/iteratoriterator

error: command failed with exit status: 1

my host compiler is

Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: arm64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

You probably still have experimental/{algorithm, coroutine, functional} in your local install dir.

Thanks!

[yronglin]

Remove fwd/repeat_view.h, and move is_repeat_specialization into __ranges/repeat_view.h

[yronglin]

Fix CI.

[yronglin]

Try fix ci

[yronglin]

libcxx/include/__ranges/repeat_view.h

112

@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

[yronglin]

fix test

[yronglin]

ping~

[philnik]

libcxx/docs/Status/Cxx2bPapers.csv

80

Please add a release note.

libcxx/include/__ranges/repeat_view.h

111

This shouldn't be a problem anymore, since the workaround is removed, right?

112

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?

139

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?

libcxx/include/__ranges/take_view.h

34

It seems you haven't don this?

libcxx/test/libcxx/ranges/range.factories/range.repeat.view/ctor.value.bound.verify.cpp

20

Why do you have an extra scope around this?

libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp

29

You don't need a main in a .compile.pass.cpp test.

libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp

18

You seem to be missing tests for the copy_constructible constraint.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/increment.pass.cpp

32–33

This seems redundant, since you already check for the specific type below.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp

58–62

This seems to not be done.

libcxx/utils/generate_feature_test_macro_components.py

569

Is something missing from the Paper? You claim it's complete in the status paper.