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 should be generic over the integer #22716

Closed
vks opened this Issue Feb 23, 2015 · 13 comments

Comments

Projects
None yet
8 participants
@vks
Copy link
Contributor

vks commented Feb 23, 2015

Currently enumerate uses usize for counting. This is necessary when iterating over slices (probably the most common scenario). However, in general an iterator is not necessarily related to memory, so it may make more sense to use other integer types.

It seems sensible to make enumerate generic over the integer type used for counting. This is backwards compatible, so it shouldn't break existing code.

pub struct Enumerate<I, T: Int> {
    iter: I,
    count: T
}

Would it be possible to make this change although enumerate is marked as stable?

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Feb 23, 2015

nominating, like we've been doing for possible backompat-libs issue

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Feb 23, 2015

Isn't it likely to break code where the integer type can't be inferred?

@utkarshkukreti

This comment has been minimized.

Copy link
Contributor

utkarshkukreti commented Feb 24, 2015

A little off-topic, but doesn't (0..).zip(_) now do the exact same thing as .enumerate()? We can just zip with 0u8.. to do enumerate() with u8.

fn main() {
    for pair in "foo".chars().enumerate() {
        println!("{:?}", pair);
    }

    for pair in (0..).zip("foo".chars()) {
        println!("{:?}", pair);
    }
}

=>

(0, 'f')
(1, 'o')
(2, 'o')
(0, 'f')
(1, 'o')
(2, 'o')
@vks

This comment has been minimized.

Copy link
Contributor Author

vks commented Feb 24, 2015

Python has both of them as well, I think it is common enough to warrant its
own iterator.

On Tue, Feb 24, 2015, 09:30 Utkarsh Kukreti notifications@github.com
wrote:

A little off-topic, but doesn't (0..).zip(_) now do the exact same thing
as .enumerate()? We can just zip with 0u8.. to do enumerate() with u8.

fn main() {
for pair in "foo".chars().enumerate() {
println!("{:?}", pair);
}

for pair in (0..).zip("foo".chars()) {
    println!("{:?}", pair);
}

}

=>

(0, 'f')
(1, 'o')
(2, 'o')
(0, 'f')
(1, 'o')
(2, 'o')


Reply to this email directly or view it on GitHub
#22716 (comment).

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Feb 26, 2015

E-needs-decision. cc @aturon 1.0 beta P-backcompat-libs.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 28, 2015

Personally I'm inclined to keep things simple and just use usize here. In particular, to do otherwise would require basically adding an associated Index type to the Iterator trait, because we must have some way for generic wrappers like Map to reflect the index type of the things they are iterating over.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 28, 2015

(The fact that zipping with 0.. provides an easy way to use other kinds of integers if you prefer is nice, though.)

@vks

This comment has been minimized.

Copy link
Contributor Author

vks commented Feb 28, 2015

Then I would honestly rather remove enumerate than having a crippled version.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Mar 2, 2015

itertools already has a generic .enumerate_from(x) (which is basically the same as zipping with x..), you could possibly import that into libcore. Specifying the starting point adds another degree of customization and an obvious place to specify the integer type. For a generic .enumerate() I would worry it's too confusing and nonobvious how to specify the integer type to use.

Standard .enumerate() is not crippled just because it's simple. It's one of a million possible iterator adaptors, and not all of them are going to be included in Rust by default.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 19, 2015

I personally see enumerate as a common iterator adaptor which is expected in a library of iterators, so I would not want to remove it in favor of the less intuitive foo.zip(0..). I think it's fine to have the open ended range to count with an arbitrary integer, but I don't think we need to change enumerate as-is.

@vks

This comment has been minimized.

Copy link
Contributor Author

vks commented Mar 19, 2015

Well, then let's at least mention zip(0..) in enumerate's documentation
as the more flexible variant.

On Thu, Mar 19, 2015, 04:03 Alex Crichton notifications@github.com wrote:

I personally see enumerate as a common iterator adaptor which is expected
in a library of iterators, so I would not want to remove it in favor of the
less intuitive foo.zip(0..). I think it's fine to have the open ended
range to count with an arbitrary integer, but I don't think we need to
change enumerate as-is.


Reply to this email directly or view it on GitHub
#22716 (comment).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 24, 2015

Ok after some more discussion the conclusion is that we probably don't want to make enumerate generic over integer types due to worries about inference and overcomplexity. This does seems like a worthwhile A-docs bug though, so I'm going to apply that label to ensure that we mention the zip alternative in the docs.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 24, 2015

triage: P-high ()

@alexcrichton alexcrichton removed this from the 1.0 beta milestone Mar 24, 2015

steveklabnik added a commit to steveklabnik/rust that referenced this issue Mar 27, 2015

steveklabnik added a commit to steveklabnik/rust that referenced this issue Mar 27, 2015

bors added a commit that referenced this issue Mar 28, 2015

@bors bors closed this in #23789 Mar 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.