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

[yronglin]

Summary

• Implement https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2474r2.html
• Implement LWG3875(https://cplusplus.github.io/LWG/issue3875).

Depends on D151629

[yronglin]

Rebase

[yronglin]

Try fix ci

[yronglin]

Fix ci

[yronglin]

Fix ci

[yronglin]

Update comments in test

[var-const]

libcxx/include/__fwd/repeat_view.h

45 ↗

(On Diff #529949)

Does __fn have to be here and not in the main repeat_view.h file? It's not a forward declaration, it's a definition.

libcxx/include/__ranges/drop_view.h

310

I think staying close to the standard is preferable here (having to use friends is a little unfortunate, but seems like the right tradeoff).

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

45

Seems not done -- I'd prefer the two test cases to only differ in whether the rv variable is const or not, but otherwise use same values (0 and 10 in this case).

55

Seems not done.

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

31

Quick question, which file contains the newly-added test case?

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

18

Nit: s/bellow/below/.

47

Can this check be ==? If I'm reading the wording correctly, if index-type is signed, then difference_type is the same type as index-type, so std::int8_t in this case?

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

24

Still not done -- I think you can completely remove next from this file and just use arithmetic operations on iterators.

56

This is brittle. Can we check against something like ptrdiff_t instead of "concrete" types? If not, I'd rather remove the check than do platform-specific definitions.

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

21

You can probably remove this next (similarly in other iterator test files).

30

Missing calls to test().

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

37

Can you also check the value of iter?

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

41

For consistency, please make sure we have the same coverage as various other adaptor.pass.cpp test files. See e.g. test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp and make sure all the relevant test cases from that file are also applied to repeat_view.

[yronglin]

Rebase and apply P2494R2

[yronglin]

Merge fwd/repeat_view.h and ranges/repeat_view.h, and address comments.

[yronglin]

Thanks for your review @var-const !

libcxx/include/__fwd/repeat_view.h

45 ↗

(On Diff #529949)

Yeah, because in drop_view.h and take_view.h called views::repeat(...), should we merge this file and the main repeat_view.h ?

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

31

In star.pass.cpp, I'd be glad to add a range-based-for in this file?

for (const auto &v : repeat_view) {
   assert(v == some_value);
}

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

24

sorry, I missed it earlier

56

I agree ptrdiff_t is better

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

41

Okay, I have add test for repeat_view piping with adaptors, except join_view.

[yronglin]

Try fix ci

[yronglin]

fix ci

[yronglin]

Fix ci

[yronglin]

libcxx/include/__ranges/take_view.h

34

@philnik I have tried to forward-declare repeat_view in __fwd/repeat_view.h before Diff 43, but in the standard, it uses views::repeat rather than repeat_view in take_view.h and drop_view.h, so we have to definition but not forward-declaration the __cpo struct in __fwd/repeat_view.h, and I have merged __fwd/repeat_view.h into the main __ranges/repeat_view.h. If this was not a correct approach, I'm glad to split a forward-declaration file again.

[yronglin]

friendly ping~

[var-const]

In D141699#4485284, @yronglin wrote:

friendly ping~

I'll take a look today or tomorrow, real sorry about the slow review!

[yronglin]

In D141699#4490495, @var-const wrote:

In D141699#4485284, @yronglin wrote:

friendly ping~

I'll take a look today or tomorrow, real sorry about the slow review!

Thanks a lot and no worries about that!

[var-const]

LGTM with the remaining comments addressed (and a green CI). Feel free to ping me directly if you have any questions re. my comments. Thanks!

libcxx/include/__fwd/repeat_view.h

45 ↗

(On Diff #529949)

Honestly, I don't have a strong preference here. @philnik What do you think? I don't think we had a precedent for defining the function objects in a fwd file (and IMO it makes it not really a forward file), so it feels perhaps a little "inelegant" (to me). Do you think this results in a meaningful reduction of compilation times?

libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp

183

Ultranit: s/return/returns/ (here and below, and same in the range.drop test)

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

19

For each test case, can you add a very short comment to explain which case it's testing?

{ // `(value)` constructor, non-const view
  std::ranges::repeat_view<int> rv(0);
  std::same_as<std::ranges::iterator_t<decltype(rv)>> decltype(auto) iter = rv.begin();
  assert(*iter == 0);
}
{ // `(value)` constructor, const view
  const std::ranges::repeat_view<int> rv(0);
  std::same_as<std::ranges::iterator_t<decltype(rv)>> decltype(auto) iter = rv.begin();
  assert(*iter == 0);
}
// ...

30

Hmm, does this case differ from std::ranges::repeat_view<int> rv(0); other than in which value is being repeated? (same for the const version below)

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

31

Yes, can you please add the test case to star.pass.cpp?

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

26

This next can be replaced by arithmetics.

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

41

I think testing with just a couple of different adaptors (e.g. drop and transform) is completely sufficient -- I'd remove the rest because it will be hard to maintain going forward.

102

There is no need to check all the possible view combinations, it would be hard to maintain and I don't think it meaningfully increases test coverage.

[philnik]

libcxx/include/__fwd/repeat_view.h

45 ↗

(On Diff #529949)

I don't think it worth it. We might consider this a problem once we remove transitive includes, but until then any avoided includes should be kept to the big gains IMO. I really hope we will do a big cleanup soon. Looking at the amount of includes removed from old standards in this and the last release makes it look like it's just a matter of time before we have to anyways.

[philnik]

I'm trusting @var-const here. I don't really have the time to do another deep dive here.

[yronglin]

Address comments

[yronglin]

Thanks for your review! @var-const @philnik

libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp

183

Thanks! done.

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

19

done

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

31

Thanks, added!

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

26

Sorry, somehow, I have missed this before.

[Mordante]

I mainly skimmed over it. I trust @philnik and @var-const to have done a deeper review; they are more familiar with the ranges details.

[yronglin]

NFC, Use + instead of std::ranges::next in tests.

[yronglin]

In D141699#4510812, @Mordante wrote:

I mainly skimmed over it. I trust @philnik and @var-const to have done a deeper review; they are more familiar with the ranges details.

Thanks a lot for your review!

[yronglin]

Waiting for CI green

[yronglin]

NFC, fix ci, use _LIBCPP_ENABLE_DEBUG_MODE instead of _LIBCPP_ENABLE_ASSERTIONS

[var-const]

In D141699#4513287, @yronglin wrote:

NFC, fix ci, use _LIBCPP_ENABLE_DEBUG_MODE instead of _LIBCPP_ENABLE_ASSERTIONS

Rather than explicitly enabling debug mode (which will fail in the hardened mode test), please add // UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode

[yronglin]

Address comments

[yronglin]

In D141699#4513484, @var-const wrote:

In D141699#4513287, @yronglin wrote:

NFC, fix ci, use _LIBCPP_ENABLE_DEBUG_MODE instead of _LIBCPP_ENABLE_ASSERTIONS

Rather than explicitly enabling debug mode (which will fail in the hardened mode test), please add // UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode

Thanks for your advice, fixed.

[yronglin]

Rebase