Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enumerate with arbitrary starting index #1800

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

serpent7776
Copy link
Contributor

Improves enumerate so that it's possible to start at any given index. Similar functionality exists in boost::adaptors::indexed

This is just a draft, it just adds an additional parameter to enumerate_fn::operator(). It works well when used with function call syntax, but:

  • doesn't work with pipe syntax without breaking existing user code (big issue)
  • increases size of index_view (not sure if an issue)
  • breaks existing ABI of enumerate (probably not an issue)

Another way to implement this would be to transform constructed zip, but I'm not sure that's any better (also doesn't fix the first issue).

@JohelEGP
Copy link
Contributor

JohelEGP commented Nov 3, 2023

doesn't work with pipe syntax without breaking existing user code (big issue)

How come?

increases size of index_view (not sure if an issue)
breaks existing ABI of enumerate (probably not an issue)

You can avoid those.

For both, you can have enumerate(range, size) return drop(enumerate(range), size).

For only the first one, you can parameterize the size.
Default the size to std::integral_constant<std::size_t, 0> and
store it in a base or use [[no_unique_address]].

@serpent7776
Copy link
Contributor Author

How come?

When I tried modifying the last test to use enumerate with custom index start I got compilation error

enumerate.cpp:157:29: error: no match for call to ‘(const ranges::views::view_closure<ranges::views::enumerate_fn>) (int)’                                                   
  157 |           | views::enumerate(99); 

I'm assuming this is because enumerate is defined to be view_closure that doesn't accept parameters on operator():

RANGES_INLINE_VARIABLE(view_closure<enumerate_fn>, enumerate)

I could change that to

RANGES_INLINE_VARIABLE(enumerate_fn, enumerate)

and add overload operator()(size_t n = 0) to enumerate_fn, but that breaks existing code, because range | enumerate no longer works and range | enumerate() is required.

For both, you can have enumerate(range, size) return drop(enumerate(range), size).

This is interesting, but I'm assuming you meant drop(detail::index_view<S, D>(), n)

For only the first one, you can parameterize the size.

I don't think I understand.

@JohelEGP
Copy link
Contributor

JohelEGP commented Nov 4, 2023

For only the first one, you can parameterize the size.

I don't think I understand.

I meant something similar to how the bound of std::ranges::iota_view defaults to std::unreachable_sentinel_t.
But in this case, you'd be defaulting the start index to std::integral_constant<std::ranges::range_difference_t<R>, 0>.

@serpent7776
Copy link
Contributor Author

I updated the code, so that it works with both function call syntax and piping syntax. I looked at existing code in split_when.hpp.

@serpent7776
Copy link
Contributor Author

I'd like to ask if the existing solution is OK or should I go with eliminating the size overhead of index_view

@serpent7776
Copy link
Contributor Author

ping @JohelEGP ?

@JohelEGP

This comment was marked as duplicate.

@JohelEGP
Copy link
Contributor

JohelEGP commented Dec 8, 2023

I think you want feedback from a maintainer now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants