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

Change VecMap's iterators to use wrapper structs instead of typedefs. #19720

Merged
merged 1 commit into from
Dec 17, 2014

Conversation

csouth3
Copy link
Contributor

@csouth3 csouth3 commented Dec 11, 2014

Using a type alias for iterator implementations is fragile since this
exposes the implementation to users of the iterator, and any changes
could break existing code.

This commit changes the iterators of VecMap to use
proper new types, rather than type aliases. However, since it is
fair-game to treat a type-alias as the aliased type, this is a:

[breaking-change].

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 11, 2014

If changes like this are desired for the other maps that use typedefs currently (like BTreeMap, TreeMap, and TrieMap), I'd be happy to do the same for those as well.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 11, 2014

wouldn't a newtype be more correct here?
once trait forwarding lands this would result in less code-noise

@Gankra
Copy link
Contributor

Gankra commented Dec 11, 2014

It's not clear that we'll be getting trait forwarding any time soon, if ever. As such this is perfectly fine.

@Gankra
Copy link
Contributor

Gankra commented Dec 11, 2014

Would it be possible for these to implement DoubleEnded as well? The Vec iterator is ExactSize, so I believe it should be DoubleEnded for these adaptors. If not we should consider a set of adaptors that do permit DoubleEnded iteration.

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 11, 2014

So after looking closer at iter::Map, it implements DoubleEndedIterator, ExactSizeIterator, and RandomAccessIterator, so I could definitely implement at least DoubleEndedIterator if not the other two as well for keys and values. Also, iter::FilterMap implements DoubleEndedIterator as well, so I could implement that for MoveItems as well.

#[inline]
fn size_hint(&self) -> (uint, Option<uint>) {
self.iter.size_hint()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also as a minor style point I tend to "squash" impl-forwarding methods like these to reduce the noise. That is:

impl<'a, V> Iterator<uint> for Keys<'a, V> {
     #[inline] fn next(&mut self) -> Option<uint> { self.iter.next() }
     #[inline] fn size_hint(&self) -> (uint, Option<uint>) { self.iter.size_hint() }
}

Because it's code that doesn't really "matter". Just trivial boiler-plate.

This is a personal thing though, so I won't block this on it. No real convention. See e.g. btreemap for an actual example of this in our codebase, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also prefer that we don't #[inline] these. Want to start cracking down on blind inlining.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in the process of adding DoubleEnded etc anyway, so I'll just squash these down while I'm in here!

@csouth3 csouth3 force-pushed the vecmap-newtypes branch 3 times, most recently from e3c5725 to 69d113f Compare December 11, 2014 18:27
@csouth3
Copy link
Contributor Author

csouth3 commented Dec 11, 2014

@gankro Documentation is fixed.

brson added a commit to brson/rust that referenced this pull request Dec 12, 2014
As mentioned in rust-lang#19663, it is more desirable long-term that iterators be implemented as wrapper structs instead of typedefs.  This pull request converts `VecMap` to the preferred solution.
@tbu-
Copy link
Contributor

tbu- commented Dec 13, 2014

@csouth3 Answering your question, this would be nice for all collections, including BTreeMap, etc.

Using a type alias for iterator implementations is fragile since this
exposes the implementation to users of the iterator, and any changes
could break existing code.

This commit changes the iterators of `VecMap` to use
proper new types, rather than type aliases.  However, since it is
fair-game to treat a type-alias as the aliased type, this is a:

[breaking-change].
@csouth3
Copy link
Contributor Author

csouth3 commented Dec 14, 2014

@gankro Got wrecked by the closure unboxing pull that just got merged, hah.
I've now rebased.

bors added a commit that referenced this pull request Dec 14, 2014
Change `VecMap`'s iterators to use wrapper structs instead of typedefs.

Reviewed-by: Gankro
bors referenced this pull request Dec 16, 2014
This creates an enormous amount of spew.
bors added a commit that referenced this pull request Dec 16, 2014
Change `VecMap`'s iterators to use wrapper structs instead of typedefs.

Reviewed-by: Gankro
@csouth3
Copy link
Contributor Author

csouth3 commented Dec 16, 2014

@gankro Is there anything I need to do to get this moved out of "BAD"? This took the fall for the auto rollup but it wasn't the offending PR.

@tbu-
Copy link
Contributor

tbu- commented Dec 16, 2014

@csouth3 Ping @gankro, like you did.

@Gankra
Copy link
Contributor

Gankra commented Dec 16, 2014

Sorry, saw this on my phone and promptly forgot about it 😅

bors added a commit that referenced this pull request Dec 17, 2014
Change `VecMap`'s iterators to use wrapper structs instead of typedefs.

Reviewed-by: Gankro, Gankro
@csouth3
Copy link
Contributor Author

csouth3 commented Dec 17, 2014

Brought down by the cruel auto-rollup again :^)

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 17, 2014
Using a type alias for iterator implementations is fragile since this
exposes the implementation to users of the iterator, and any changes
could break existing code.

This commit changes the iterators of `VecMap` to use
proper new types, rather than type aliases.  However, since it is
fair-game to treat a type-alias as the aliased type, this is a:

[breaking-change].
@bors bors merged commit 81f9a31 into rust-lang:master Dec 17, 2014
@csouth3 csouth3 deleted the vecmap-newtypes branch December 18, 2014 01:11
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.

7 participants