Page MenuHomePhabricator

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

Authored by yronglin on Jan 13 2023, 8:28 AM.

Diff Detail

Unit TestsFailed

TimeTest
4,200 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.libcxx/ranges/range_factories/range_repeat_view::ctor.value.bound.verify.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/50a70c54d0fa-1/llvm-project/libcxx-ci/libcxx/test/libcxx/ranges/range.factories/range.repeat.view/ctor.value.bound.verify.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/50a70c54d0fa-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/50a70c54d0fa-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/50a70c54d0fa-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_ENABLE_ASSERTIONS=1 -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
12,850 mslibcxx CI Modular build > llvm-libc++-shared-cfg-in.libcxx/ranges/range_factories/range_repeat_view::ctor.value.bound.verify.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/libcxx/test/libcxx/ranges/range.factories/range.repeat.view/ctor.value.bound.verify.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fmodules -fcxx-modules -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_ENABLE_ASSERTIONS=1 -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
580 mslibcxx CI Modular build > llvm-libc++-shared-cfg-in.std/ranges/range_factories/range_repeat_view::ctad.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fmodules -fcxx-modules -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only
720 mslibcxx CI Modular build > llvm-libc++-shared-cfg-in.std/ranges/range_factories/range_repeat_view::ctor.value.bound.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fmodules -fcxx-modules -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/test/std/ranges/range.factories/range.repeat.view/Output/ctor.value.bound.pass.cpp.dir/t.tmp.exe
680 mslibcxx CI Modular build > llvm-libc++-shared-cfg-in.std/ranges/range_factories/range_repeat_view::views_repeat.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fmodules -fcxx-modules -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/c21d5090d5ad-1/llvm-project/libcxx-ci/build/generic-modules/test/std/ranges/range.factories/range.repeat.view/Output/views_repeat.pass.cpp.dir/t.tmp.exe
View Full Test Results (12 Failed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
philnik added inline comments.Fri, Mar 3, 9:00 AM
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.

This revision now requires changes to proceed.Fri, Mar 3, 9:00 AM
yronglin updated this revision to Diff 502419.EditedSun, Mar 5, 3:19 AM
yronglin marked 43 inline comments as done.

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 updated this revision to Diff 502420.Sun, Mar 5, 3:35 AM

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?

philnik added inline comments.Sun, Mar 5, 4:33 AM
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 added inline comments.Sun, Mar 5, 7:00 AM
libcxx/include/__ranges/repeat_view.h
112

Thanks, I will try to remove this workaround.

yronglin updated this revision to Diff 502434.Sun, Mar 5, 7:46 AM

Remove workaround.

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

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.

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 updated this revision to Diff 502437.Sun, Mar 5, 8:58 AM

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

yronglin updated this revision to Diff 502620.Mon, Mar 6, 6:27 AM

Try fix ci

yronglin marked an inline comment as done.Mon, Mar 6, 8:47 AM
yronglin added inline comments.
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

philnik added inline comments.Mon, Mar 27, 2:28 AM
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.