Skip to content

Adding position to Enumerate #435

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

Closed
x4exr opened this issue Sep 2, 2024 · 18 comments
Closed

Adding position to Enumerate #435

x4exr opened this issue Sep 2, 2024 · 18 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@x4exr
Copy link

x4exr commented Sep 2, 2024

Proposal

Adding position function to Enumerate struct.

Problem statement

Other than doing next() which will progress the position, there is no straight forward way to get the position. If you call something that uses your iterator, you cant know how far it progressed without knowing the position start and end.

Motivating examples or use cases

Solution sketch

    #[inline]
    pub fn position(&self) -> usize {
        self.count
    }

Alternatives

Putting Peek around Enumerate, then doing .peek() and subtracting 1 from the position. The issue with this is if you are at the end of the iteration, you get None and need to store the end position else where. This also uses peek in a way that is unnatural and unneccesary. Its over complicated

Links and related work

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@x4exr x4exr added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Sep 2, 2024
@x4exr x4exr changed the title (My API Change Proposal) Adding position to Enumerate Sep 2, 2024
@pitaj
Copy link

pitaj commented Sep 2, 2024

@scottmcm
Copy link
Member

scottmcm commented Sep 2, 2024

@x4exr
Copy link
Author

x4exr commented Sep 3, 2024

So could offset also get added to enumerated then

@workingjubilee
Copy link
Member

Putting Peek around Enumerate, then doing .peek() and subtracting 1 from the position. The issue with this is if you are at the end of the iteration, you get None and need to store the end position else where. This also uses peek in a way that is unnatural and unneccesary. Its over complicated

You would have to subtract 1 anyways, because the initial position will actually be an item you haven't iterated to yet.

@workingjubilee
Copy link
Member

workingjubilee commented Sep 3, 2024

@x4exr Do you have a practical use-case in mind? It's not entirely clear if you want what you say you want, because of the whole "the actual first position will be 0, even before you call .next()" problem.

@x4exr
Copy link
Author

x4exr commented Sep 3, 2024

Yes I need this method to measure the number of iterations made when I call a function such as parse string with Mt iterator inside

@workingjubilee
Copy link
Member

workingjubilee commented Sep 3, 2024

Ah, so you're going to call... hm, it can't be position, and it can't be count, so let's say iter.current_count() (or .offset() maybe), and then you're going to call it again later, and subtract the two? So kind of like this?

let start = iter.current_count();
my_parser_that_accepts_iterators(iter);
let end = iter.current_count();
let iterations = end - start;

@x4exr
Copy link
Author

x4exr commented Sep 4, 2024

Yes, I was even thinking enumerated_position

@pitaj
Copy link

pitaj commented Sep 4, 2024

Another option that avoids the conflicts entirely, meaning you can use any name you want:

    #[inline]
    pub fn count(this: &Self) -> usize {
        this.count
    }

@x4exr
Copy link
Author

x4exr commented Sep 13, 2024

why call it this? is it so rust does not accept this as a method? Also .count() already is a function in Iterator

@pitaj
Copy link

pitaj commented Sep 13, 2024

Yes, it doesn't become a method, so there's no name conflict. It's called like Enumerate::count(&instance)

@Amanieu
Copy link
Member

Amanieu commented Sep 17, 2024

We discussed this in the libs-api meeting and are happy to accept this. However we feel that the name next_index is clearer since it describes exactly what this method is returning: the index that will be associated with the next element returned by next.

@Amanieu Amanieu closed this as completed Sep 17, 2024
@Amanieu Amanieu added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Sep 17, 2024
@kennytm
Copy link
Member

kennytm commented Sep 18, 2024

While next_index() is very clear on its own, together with other existing iterator functions like next() / next_back() / next_chunk() / next_if() it sounds like calling it will mutate the iterator.

@pitaj
Copy link

pitaj commented Sep 18, 2024

What about peek_index?

@x4exr
Copy link
Author

x4exr commented Sep 18, 2024

What does it have to do with peek?

@cuviper
Copy link
Member

cuviper commented Sep 18, 2024

It's a preview of what next() would return, similar to Peekable::peek().

@Amanieu
Copy link
Member

Amanieu commented Sep 18, 2024

While next_index() is very clear on its own, together with other existing iterator functions like next() / next_back() / next_chunk() / next_if() it sounds like calling it will mutate the iterator.

We did discuss this issue in the meeting but we felt like this was unlikely to be an issue in practice. While it follows the same pattern as the next_* methods, it's unlikely that this would be confused with advancing the iterator. The documentation would make that clear, especially with the method taking &self.

@workingjubilee
Copy link
Member

We'll have plenty of time to argue over what Pantone chips to use, let's just get it on nightly.

ChrisDenton added a commit to ChrisDenton/rust that referenced this issue Apr 19, 2025
add next_index to Enumerate

Proposal: rust-lang/libs-team#435
Tracking Issue: rust-lang#130711

This basically just reopens rust-lang#130682 but squashed and with the new function and the feature gate renamed to `next_index.`

There are two questions I have already:
- Shouldn't we add test coverage for that? I'm happy to provide some, but I might need a pointer to where these test would be.
  - Maybe I could actually also add a doctest?
- For now, I just renamed the feature name in the unstable attribute to `next_index`, as well, so it matches the new name of the function. Is that okay? And can I just do that and use any string, or is there a sealed list of features defined somewhere where I also need to change the name?
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 20, 2025
Rollup merge of rust-lang#139533 - jogru0:130711, r=Mark-Simulacrum

add next_index to Enumerate

Proposal: rust-lang/libs-team#435
Tracking Issue: rust-lang#130711

This basically just reopens rust-lang#130682 but squashed and with the new function and the feature gate renamed to `next_index.`

There are two questions I have already:
- Shouldn't we add test coverage for that? I'm happy to provide some, but I might need a pointer to where these test would be.
  - Maybe I could actually also add a doctest?
- For now, I just renamed the feature name in the unstable attribute to `next_index`, as well, so it matches the new name of the function. Is that okay? And can I just do that and use any string, or is there a sealed list of features defined somewhere where I also need to change the name?
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Apr 20, 2025
add next_index to Enumerate

Proposal: rust-lang/libs-team#435
Tracking Issue: #130711

This basically just reopens #130682 but squashed and with the new function and the feature gate renamed to `next_index.`

There are two questions I have already:
- Shouldn't we add test coverage for that? I'm happy to provide some, but I might need a pointer to where these test would be.
  - Maybe I could actually also add a doctest?
- For now, I just renamed the feature name in the unstable attribute to `next_index`, as well, so it matches the new name of the function. Is that okay? And can I just do that and use any string, or is there a sealed list of features defined somewhere where I also need to change the name?
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this issue Apr 22, 2025
add next_index to Enumerate

Proposal: rust-lang/libs-team#435
Tracking Issue: rust-lang#130711

This basically just reopens rust-lang#130682 but squashed and with the new function and the feature gate renamed to `next_index.`

There are two questions I have already:
- Shouldn't we add test coverage for that? I'm happy to provide some, but I might need a pointer to where these test would be.
  - Maybe I could actually also add a doctest?
- For now, I just renamed the feature name in the unstable attribute to `next_index`, as well, so it matches the new name of the function. Is that okay? And can I just do that and use any string, or is there a sealed list of features defined somewhere where I also need to change the name?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

7 participants