Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
yronglin updated this revision to Diff 529787.Jun 8 2023, 5:11 PM

Try fix ci

yronglin updated this revision to Diff 529949.Jun 9 2023, 6:57 AM

Update comments in test

var-const added inline comments.Jun 23 2023, 11:19 AM
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 updated this revision to Diff 534338.Jun 25 2023, 8:09 AM

Rebase and apply P2494R2

yronglin updated this revision to Diff 534364.Jun 25 2023, 10:52 AM
yronglin marked 13 inline comments as done.

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

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 updated this revision to Diff 534491.Jun 26 2023, 3:58 AM

Try fix ci

yronglin added inline comments.Jun 28 2023, 7:34 AM
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.

friendly ping~

friendly ping~

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

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 accepted this revision.Jul 17 2023, 3:01 PM

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 added inline comments.Jul 17 2023, 3:18 PM
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 accepted this revision.Jul 17 2023, 3:20 PM

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

yronglin updated this revision to Diff 541455.Jul 18 2023, 4:29 AM
yronglin marked 6 inline comments as done.

Address comments

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 accepted this revision.Jul 18 2023, 8:24 AM

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.

This revision is now accepted and ready to land.Jul 18 2023, 8:24 AM
yronglin updated this revision to Diff 541579.Jul 18 2023, 8:49 AM

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

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!

Waiting for CI green

yronglin updated this revision to Diff 541830.Jul 18 2023, 8:47 PM

NFC, fix ci, use _LIBCPP_ENABLE_DEBUG_MODE instead of _LIBCPP_ENABLE_ASSERTIONS

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 updated this revision to Diff 541872.Jul 19 2023, 1:07 AM

Address comments

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.

This revision was automatically updated to reflect the committed changes.