This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] P2116R9: Implements `views::enumerate`
AbandonedPublic

Authored by H-G-Hristov on Aug 5 2023, 7:33 AM.

Details

Reviewers
var-const
Group Reviewers
Restricted Project
Summary

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Rebased + added iterator base test

Added three way compare test

Rebased + cleanup compare.three_way test

Rebased + cleanup

H-G-Hristov published this revision for review.Aug 21 2023, 6:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 6:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for working on this!
I mainly glossed over the patch and didn't do a real review. Just a few comments.

libcxx/include/__ranges/concepts.h
151 ↗(On Diff #551923)

This is already implied by the __uglified name. The same at other places.

libcxx/include/__ranges/enumerate_view.h
65

Can you test whether this helps clang-format to do slightly better?

Zingam added a subscriber: Zingam.Aug 25 2023, 7:32 AM

Just a note to me. Adjust modules according to this: https://reviews.llvm.org/D158358

Addressed some comments

H-G-Hristov marked an inline comment as done.Sep 4 2023, 12:19 AM

Thank you for the comments!

libcxx/include/__ranges/enumerate_view.h
65

It doesn't make a difference. I don't like how requires is snapped to the brackets () but I'll add it for consistency with the rest of the code.
I don't think that clang-format did that bad. Where else will = default or {} go without making it even less readable?

Addressed more comments

Try to fix CI

H-G-Hristov marked an inline comment as done.Sep 4 2023, 8:23 AM
var-const requested changes to this revision.Sep 8 2023, 9:19 PM
var-const added a subscriber: var-const.

Thank you for the patch! All in all it looks pretty good already. I have only started going through tests but wanted to share the feedback I already have.

libcxx/docs/Status/Cxx2cIssues.csv
12

IMO this should be Complete as well (I think Nothing To Do is for things like semantic requirements that are not enforceable in code).

libcxx/docs/Status/RangesViews.csv
38

Nit: please add a link to the patch (replacing the No patch yet part).

libcxx/include/__ranges/concepts.h
146 ↗(On Diff #556109)

Do we need this annotation? Where is it coming from?

149 ↗(On Diff #556109)

Since it's only used in enumerate_view, I would move the concept to that header (that's what we do for other view-specific concepts).

libcxx/include/__ranges/enumerate_view.h
49

Nit: I don't think the , class template enumerate_view::iterator part adds much value, it just restates the declaration that immediately follows. I'd remove the whole comment, but if you'd prefer to keep it, then let's keep just the section name.

57

Rather than making this public, can we use friends?

153

Do we need to move __pos (IIUC, it's always an integral type)?

161

Nit: extraneous semicolon.

305

Optional: rather than turning off clang-format, you could use comments to force the text to align (the upside is not having to manually do or maintain other aspects of formatting). E.g. as_rvalue_view does this:

template <class _Range>
_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range) const
    noexcept(noexcept(/**/ as_rvalue_view(std::forward<_Range>(__range))))
        -> decltype(/*--*/ as_rvalue_view(std::forward<_Range>(__range))) {
  return /*-------------*/ as_rvalue_view(std::forward<_Range>(__range));
libcxx/include/ranges
320

I think we need to remove the freestanding comments since we don't implement freestanding yet.

libcxx/test/std/ranges/range.adaptors/range.enumerate/adaptor.pass.cpp
34–36

Nit: formatting around here looks a little surprising.

41

Can this just be a call to ranges::equal?

76

Nit: please add a blank line after this line.

libcxx/test/std/ranges/range.adaptors/range.enumerate/base.pass.cpp
31

Nit: can we add a test case that uses a non-const view (without moving)?

36

Nit: please add a blank line between different test cases. Applies throughout the patch.

libcxx/test/std/ranges/range.adaptors/range.enumerate/begin.pass.cpp
17

Nit: this comment should contain just the function declaration, without the definition. Applies throughout.

30

Can we check both the case of iterator<false> and of iterator<true>?

36

Nit: I think we've been moving away from using ASSERT_SAME_TYPE towards just doing a static_assert directly.

55

Ultranit: s/a/an/.

libcxx/test/std/ranges/range.adaptors/range.enumerate/enable_borrowed_range.compile.pass.cpp
17

Nit: please remove the freestanding bit (throughout).

libcxx/test/std/ranges/range.adaptors/range.enumerate/end.pass.cpp
44

We also need to check the case where end returns a sentinel, and both the const and non-const cases.

46

s/begin/end/.

51

Should be .end().

libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h
19

Does this need to be a view? If it does, then I think it should be named View and not just Range.

39

When accessing internal types, we need to use _LIBCPP_STATIC_ASSERT. That macro is a no-op when tests are run by other implementations (e.g. MSVC) -- our test suite is used by other implementations as well.

42

Can we static_assert that this class satisfies sized_range?

This revision now requires changes to proceed.Sep 8 2023, 9:19 PM

Addressed some comments

H-G-Hristov marked 11 inline comments as done.Sep 11 2023, 5:32 AM

Thank you for the patch! All in all it looks pretty good already. I have only started going through tests but wanted to share the feedback I already have.

Thank you for the review!

Addressed more review comments + added a missing test

H-G-Hristov marked 6 inline comments as done.Sep 11 2023, 8:29 AM
H-G-Hristov added inline comments.
libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/equal.pass.cpp
25

It looks like I forgot to add the this test.

H-G-Hristov marked an inline comment as not done.Sep 11 2023, 8:31 AM

Addressed more review comments

Added operator== test

H-G-Hristov marked 3 inline comments as done.

Addressed a couple of comments

H-G-Hristov marked an inline comment as done.Sep 11 2023, 9:44 PM

Finished going through the tests -- some more feedback!

libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/equal.pass.cpp
49

It should be possible to use types::for_each to cut down on the boilerplate a little bit (from type_algorithms.h). Applies to other similar parts of the patch as well.

libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/minus.pass.cpp
14

Nit: s/enumerate_view/enumerate_view::sentinel/?

86

Question: why does BufferView need to be a separate class? Can it be merged with Range?

94

Nit: missing include.

131

Can we also static_assert that Base is not a sized_range? (and vice versa below)

134

Optional nit: IMO it would be slightly easier to read if these were sorted, for example:

static_assert(!HasMinus<EnumerateIter<Base>, EnumerateSentinel<Base>>);
static_assert(!HasMinus<EnumerateIter<Base>, EnumerateConstSentinel<Base>>);
static_assert(!HasMinus<EnumerateConstIter<Base>, EnumerateSentinel<Base>>);
static_assert(!HasMinus<EnumerateConstIter<Base>, EnumerateConstSentinel<Base>>);

static_assert(!HasMinus<EnumerateSentinel<Base>, EnumerateIter<Base>>);
static_assert(!HasMinus<EnumerateSentinel<Base>, EnumerateConstIter<Base>>);
static_assert(!HasMinus<EnumerateConstSentinel<Base>, EnumerateIter<Base>>);
static_assert(!HasMinus<EnumerateConstSentinel<Base>, EnumerateConstIter<Base>>);

(below as well)

libcxx/test/std/ranges/range.adaptors/range.enumerate/size.pass.cpp
16

Nit: please add a semicolon at the end (so that it looks like a declaration)?

// constexpr auto size() requires sized_range<V>;
29

Can we check with a (pathological) type that is sized if it's non-const but not sized if it's const?

var-const added inline comments.Sep 12 2023, 12:00 PM
libcxx/include/__ranges/enumerate_view.h
52

Are we testing this constraint? (sorry if I missed it)

libcxx/test/std/ranges/range.adaptors/range.enumerate/ctad.compile.pass.cpp
2

Note: you might also need to add the new view to libcxx/test/std/ranges/iterator_robust_against_adl.compile.pass.cpp.

libcxx/test/std/ranges/range.adaptors/range.enumerate/ctor.base.pass.cpp
62

This seems to be testing the default constructor?

74

Nit: please add a blank line after this line to separate the test cases.

libcxx/test/std/ranges/range.adaptors/range.enumerate/ctor.default.pass.cpp
36

Nit: it seems like this view can be merged with DefaultConstructibleView and NoDefaultView can be removed altogether.

45

Nit: s/NoDefaultView/NoDefaultCtrView/?

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/arithmetic.pass.cpp
79

Optional: I think it can be just {ts, ts + 5}.

82

Nit: add a comma?

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/base.pass.cpp
16

Is this the right synopsis?

54

Can we check that the iterator was moved?

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/compare.three_way.pass.cpp
62

Nit: I don't think this check (and the helper concept) is necessary -- there are no constraints, and the compiler will complain when we try to call operator<=> anyway were it somehow not available.

79

Can we also have a similar check (but presumably evaluating to true) in the test case above?

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/ctor.convert.pass.cpp
2

We also need a types.pass.cpp or similar test file to check iterator_concept, iterator_category and other nested types of the iterator.

17

Can we check the convertible_to constraint?

51

Hmm, this seems to test operator++?

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/deref.pass.cpp
43

Note: some tests rely on CTAD when initializing std::array and some pass the template arguments explicitly. Can this be made consistent across the patch? (I would slightly prefer relying on CTAD to avoid explicitly specifying the size, but either is fine)

48

Can we also check with a const iterator?

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/equal.pass.cpp
16

Can we check the noexcept part?

36

Can we also check reverse argument order for each case, and increment it1 until it equals it2 and check that?

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/index.pass.cpp
44

Let's also check the return type and noexcept, and make sure we can call index() on a const iterator.

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/iter_move.pass.cpp
18

I don't think we're checking the is_nothrow_move_constructible_v constraint?

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/subscript.pass.cpp
48

Can you add a quick comment to explain why we can't use assert(it[0] == *it); here instead of std::get?

libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/base.pass.cpp
34

Nit: we only use this function once, consider inlining it.

libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/ctor.convert.pass.cpp
17

Can we check the convertible_to constraint?

48

s/sResult/sBase/?

libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/ctor.default.pass.cpp
16

The synopsis is for iterator, not sentinel.

24

IMO we should either test with other sentinel types or not make Sentinel a template parameter (throughout).

33

Question: why are so many calls to base necessary?

libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/equal.pass.cpp
17

Is it possible to also check the OtherConst case?

42

Can we check s == it and s != it as well?

var-const added inline comments.Sep 12 2023, 4:39 PM
libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h
61

Nit: from the names, it's not obvious what is the difference between Range (which is also a view), JustAView and MinimalView. I understand that Range has some additional testing utilities (perhaps rename it to TestView? Not a great name, but perhaps just a little more descriptive). Can you explain a bit how JustAView and MinimalView differ in case it helps us come up with some more descriptive names?

75

static_assert(!std::invocable<NotInvocable>);?

99

Optional nit: in general, I'd suggest having one static_assert per check (so that should it fire, it's more obvious which exact check failed).

102

Optional nit: the class name makes it sound like it's always noexcept. Perhaps something like MaybeNoexceptIterMoveInputIterator?

Finished going through the tests -- some more feedback!

@var-const Thank you for the review!

@H-G-Hristov Hi, do you need any help to make progress on this? We're trying to clear up our Phabricator review queue and it would be great to land this :-)

@H-G-Hristov Hi, do you need any help to make progress on this? We're trying to clear up our Phabricator review queue and it would be great to land this :-)

@ldionne I'm sorry for the delayed answer. I don't mind if any one better than me takes over to complete this patch otherwise I'll try to update the patch before the end of the month. I'll definitely need some help with a few a the comments though.
I also have this patch here: https://reviews.llvm.org/D150525 and I am not sure how to implement the NullablePointer requirement as requested in the review.

H-G-Hristov marked 4 inline comments as done.

Rebased + addressed a few comments

H-G-Hristov marked 6 inline comments as done.

Addressed a few more comments

H-G-Hristov marked 8 inline comments as done.

Addressed some more comments

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/base.pass.cpp
54

@var-const I'm not sure how to check if the iterator moved. I don't seem to find an example. Could you please give me a hint.

libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/subscript.pass.cpp
48

I guess I wanted to be explicit. I undid that.

Try to fix CI

Try to fix CI

H-G-Hristov marked an inline comment as done.

Added types.compile.pass.cpp

Rebased + Try to upload changes again

H-G-Hristov marked 2 inline comments as done.

Renamed Range

H-G-Hristov added inline comments.Oct 19 2023, 10:48 AM
libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h
19

I think I got the inspiration for the name Range from other tests. I renamed it to RangeView and derivatives. Please let me know if it better like that.

61

Renamed to MinimalDefaultConstructedView. MinimalView requires a more sophisticated initializations which isn't needed for some tests.
Is it better like that?

H-G-Hristov marked 3 inline comments as done.

Addressed more comments

H-G-Hristov marked 6 inline comments as done.

Addressed some more comments

H-G-Hristov marked an inline comment as done.

Addressed more comments

H-G-Hristov marked an inline comment as done.Oct 21 2023, 12:48 AM
H-G-Hristov marked an inline comment as done.

Updated test

libcxx/include/__ranges/enumerate_view.h
52

I added some tests to adaptor.pass.cpp. I am not entirely sure if it covers it all. Please let me know if it's sufficient.

H-G-Hristov marked an inline comment as done.

Added base.pass test

H-G-Hristov marked 2 inline comments as done.Oct 22 2023, 3:38 AM
H-G-Hristov marked an inline comment as done.

Addressed comment

H-G-Hristov added inline comments.Oct 22 2023, 3:54 AM
libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/equal.pass.cpp
49

All other tests in ranges use this pattern. I fails to see how types::for_each will improve readability. Anyway I can do it if you prefer the tests to use types::for_each.

H-G-Hristov marked an inline comment as done.

Addressed some comments