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

[H-G-Hristov]

Summary

Implements P2116R9 views::enumerate

https://wg21.link/P2164R9
https://wg21.link/range.enumerate / https://eel.is/c++draft/range.enumerate

[H-G-Hristov]

Rebased

[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?

[H-G-Hristov]

Addressed some comments

[H-G-Hristov]

In D157193#4642507, @var-const wrote:

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!

[H-G-Hristov]

Addressed more review comments + added a missing test

[H-G-Hristov]

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]

Addressed more review comments

[H-G-Hristov]

Added operator== test

[H-G-Hristov]

Addressed a couple of comments

[var-const]

Finished going through the tests -- some more feedback!

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?

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]

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

60

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?

74

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

98

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).

101

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

[H-G-Hristov]

In D157193#4644531, @var-const wrote:

Finished going through the tests -- some more feedback!

@var-const Thank you for the review!