This is an archive of the discontinued LLVM Phabricator instance.

[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
var-const added inline comments.May 31 2023, 11:37 PM
libcxx/include/__ranges/drop_view.h
279

Question: is __is_empty_view necessary here?

283

Nit: in existing files, we try to format the three expressions (the actual return expression and its two duplicates inside the noexcept and the decltype) so that all the three are aligned, making it easier to quickly check that they are exactly the same. Can you format these similarly? Feel free to disable clang-format for those lines if it gets in the way.

285

In the standard, it uses views::repeat rather than repeat_view, is there a reason not to do the same here?

285

Question: the standard instead uses __range.value_, is using begin instead deliberate?

libcxx/include/__ranges/repeat_view.h
43

Hmm, it's move_constructible in the latest draft of the Standard. Is landing https://reviews.llvm.org/D151629 a prerequisite to do that?

45

The latest draft of the Standard uses integer-like-with-usable-difference-type which seems to have a couple more conditions. If this is deliberate, can you please add a comment with a TODO?

45

Question: can _Bound be a user-defined type?

54

requires copy_constructible<T>?

57

must be greater than or equal to 0

68

Perhaps we could add an assertion for the The behavior is undefined if Bound is not unreachable_sentinel_t and bound_ is negative clause?

109

Question: this is already asserted in the constructor of the view. What is the situation where it needs to be asserted again when creating the iterator?

133

Perhaps assert on Preconditions: If Bound is not unreachable_sentinel_t, current_ > 0?

libcxx/test/libcxx/ranges/range.factories/range.repeat.view/ctor.value.bound.verify.cpp
20 ↗(On Diff #507729)

We need to check both constructors (and the iterator's constructor if we keep the assertion there).

libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp
224 ↗(On Diff #507729)

Ultranit: s/returns an/return a/.

224 ↗(On Diff #507729)

Ultranit: can you also put backticks around repeat_view and sized_range (here and in the similar comment below)? Otherwise it's a little inconsistent.

224 ↗(On Diff #507729)

Can you also add a static_assert to show that the view models sized_range (and similarly doesn't model sized_range below)?

233 ↗(On Diff #507729)

Ultranit: s/not models/doesn't model/.

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

Can you add a short comment to each test case to briefly explain what it does? It makes tests much easier to read.

45

Nit: I'd suggest s/1024/10/ here to make it more obvious that this test case checks for the same thing as the previous one but for a const repeat_view.

55

Same comment re. s/33/20/ as above.

libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp
15 ↗(On Diff #507729)

Nit: <cassert> can be removed.

21 ↗(On Diff #507729)

Ultranit: if the space between the two closing angle brackets is deliberate, then perhaps also add a space after the corresponding opening angle bracket?

25 ↗(On Diff #507729)

Nit: I'd just use Empty() for this line and the line below -- we already checked the references deduce to a non-reference type above, and these 3 lines seem focused on how the bound is deduced.

29 ↗(On Diff #507729)

Nit: this main is unnecessary.

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

These preceding underscores are unnecessary -- we only use them within the library to make sure the names of our variables/functions/etc. cannot clash with user-defined macros. It also creates an impression that the tests below are accessing some internal state.

(honestly, I'd even remove the trailing underscore but that's a matter of taste. Personally, I only use it for private variables)

24

Optional: since this struct is only used to do a compile-time check, you could remove the member variable and just have the constructor defined as Int(int) {}.

libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.piecewise.pass.cpp
80 ↗(On Diff #507729)

Can we also test that the view is not constructible when given a tuple that can be used to construct A and a tuple that cannot be used to construct the bound?

libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp
38 ↗(On Diff #507729)

Nit: consider replacing specific with user-provided, explicitly provided or similar.

58 ↗(On Diff #507729)

Nit: this e is const but similar ones below aren't, can this be consistent?

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

Nit: I think this should return unreachable_sentinel_t and be noexcept.

18

Nit: <utility> seems unused, but unreachable_sentinel_t requires <iterator>.

31

Question: do we have a test where we iterate over a whole bounded view, i.e. go from begin to end and make sure the same value is repeated?

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/compare.pass.cpp
18 ↗(On Diff #507729)

Can you add comments to each test case?

32 ↗(On Diff #507729)

I think we should also check the generated comparison operators (unless there's precedent for not doing so in existing tests).

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/ctor.default.pass.cpp
17 ↗(On Diff #507729)

Question: is the default constructor guaranteed to be nothrow?

19 ↗(On Diff #507729)

Nit: in new code, we prefer to use [[maybe_unused]] instead of a void cast.

19 ↗(On Diff #507729)

Are there any properties of a default-constructed iterator that we could check? It it perhaps equal to end()?

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/decrement.pass.cpp
22 ↗(On Diff #507729)

Rather than comparing the two iterators, consider comparing the return values to rv.begin() + some-index, i.e. something like assert(iter1-- == rv.begin() + 9);.

30 ↗(On Diff #507729)

Can you check the exact type? That would be more specific and replace all the 3 static_asserts.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/increment.pass.cpp
33 ↗(On Diff #507729)

Nit: the static assertions below that check for the exact type make these redundant.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp
53 ↗(On Diff #507729)

Would it be possible to use std::uint64_t or similar?

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus.pass.cpp
23 ↗(On Diff #507729)

(applies throughout) If the iterator is random access, can you avoid using next and just do v.begin() + 10?

26 ↗(On Diff #507729)

Nit: I don't think this check adds much value.

27 ↗(On Diff #507729)

Nit: consider using next from begin instead.

29 ↗(On Diff #507729)

Nit: if possible, it's better to check for the exact return type (applies throughout).

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus_eq.pass.cpp
23 ↗(On Diff #507729)

Same comments as the minus.pass.cpp file.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/plus.pass.cpp
26 ↗(On Diff #507729)

Same comments as for operator- -- I think it's better to compare to v.begin() + N.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/star.pass.cpp
18 ↗(On Diff #507729)

Can you add test cases to check a view that has bounds? I'd check a one-element view and a view of several elements.

23 ↗(On Diff #507729)

Nit: I don't think it's necessary to use addressof in a test, it's used to make sure we don't have to deal with an overloaded operator& which cannot be the case here.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/subscript.pass.cpp
21 ↗(On Diff #507729)

Nit: use std::ranges::all_of?

libcxx/test/std/ranges/range.factories/range.repeat.view/size.pass.cpp
11 ↗(On Diff #507729)

Nit: s/see below/(!same_as<Bound, unreachable_sentinel_t>);/.

17 ↗(On Diff #507729)

Nit: can these be sorted? (And I think also s/<utility>/<iterator>/)

libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp
11 ↗(On Diff #507729)

Nit: it would be helpful to have the constraints here as well.

40 ↗(On Diff #507729)

Please add tests for piping the view with other views, see various adaptor.pass.cpp tests for examples.

This revision now requires changes to proceed.May 31 2023, 11:37 PM
yronglin updated this revision to Diff 528486.Jun 5 2023, 9:59 AM
yronglin marked 62 inline comments as done.

Address comments.
Implement LWG3875(https://cplusplus.github.io/LWG/issue3875).

Sorry for the late reply and thanks a lot for your review! @var-const

libcxx/docs/FeatureTestMacroTable.rst
347 ↗(On Diff #507729)

This value is incorrect.

libcxx/docs/Status/Cxx2bPapers.csv
80 ↗(On Diff #507729)

done.

libcxx/include/__ranges/drop_view.h
279

We don't need __is_empty_view here, I'll remove that.

283

Thanks for your tips, add // clang format off here.

285

I used ranges::begin previously, because __range.__value_ was a private member of repeat_view. Nowadays, I make the std::views::__take::__fn and std::views::__drop::__fn as the friend of repeat_view, so we can access the private member __range.__value_. WDYT?

285

done, use views::repeat.

libcxx/include/__ranges/repeat_view.h
43

Yes!

45

LWG3875(https://cplusplus.github.io/LWG/issue3875) introduced integer-like-with-usable-difference-type , I think this patch need update to apply this change.

45

Question: can _Bound be a user-defined type?

As far as I know, _Bound can not be a user-define type.

54

The constraint requires copy_constructible<T> introduced by https://open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2494r2.html#wording-range.adjacent.transform.view . Once D151629 landed, I'll add requires copy_constructible<T>.

68

done!

109

Good catch! I think it's unnecessary, I'll remove it.

libcxx/include/__ranges/take_view.h
34

Sorry, somehow I have missed this, I've added __fwd/repeat_view.h now.

libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp
21 ↗(On Diff #507729)

Thanks, I'll remove this whitespace

29 ↗(On Diff #507729)

Thanks, removed.

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

Good catch, I'll try to add a new test case.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/compare.pass.cpp
32 ↗(On Diff #507729)

Thanks, I'll add test to check the generated comparison operators.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/ctor.default.pass.cpp
17 ↗(On Diff #507729)

Nope, I have not found any words that guaranteed the default ctor to be nothrow in the latest draft, I'll try to use std::is_default_constructible_v.

19 ↗(On Diff #507729)

IMO, if we want to check the properties of a default-constructed iterator, we can only check the __current_ = _IndexT()

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/decrement.pass.cpp
22 ↗(On Diff #507729)

agree

30 ↗(On Diff #507729)

agree

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp
53 ↗(On Diff #507729)

done

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus.pass.cpp
26 ↗(On Diff #507729)

Thanks, removed.

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/subscript.pass.cpp
21 ↗(On Diff #507729)

done!

libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp
11 ↗(On Diff #507729)

done.

libcxx/utils/generate_feature_test_macro_components.py
563

Oops, this value should be False.

yronglin edited the summary of this revision. (Show Details)Jun 5 2023, 10:15 AM
yronglin updated this revision to Diff 528776.Jun 6 2023, 3:42 AM

Try fix ci.

yronglin updated this revision to Diff 528825.Jun 6 2023, 6:16 AM

Try fix ci

yronglin updated this revision to Diff 528866.Jun 6 2023, 7:57 AM

Try fix ci

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
285

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
17 ↗(On Diff #529949)

Nit: s/bellow/below/.

46 ↗(On Diff #529949)

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
23 ↗(On Diff #507729)

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

55 ↗(On Diff #529949)

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
20 ↗(On Diff #529949)

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

29 ↗(On Diff #529949)

Missing calls to test().

libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/star.pass.cpp
36 ↗(On Diff #529949)

Can you also check the value of iter?

libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp
40 ↗(On Diff #507729)

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
23 ↗(On Diff #507729)

sorry, I missed it earlier

55 ↗(On Diff #529949)

I agree ptrdiff_t is better

libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp
40 ↗(On Diff #507729)

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 ↗(On Diff #534929)

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
25 ↗(On Diff #534929)

This next can be replaced by arithmetics.

libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp
101 ↗(On Diff #534929)

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.

40 ↗(On Diff #507729)

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.

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 ↗(On Diff #534929)

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
25 ↗(On Diff #534929)

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.