Clean up libcollections::VecMap
#19663
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Introduce a named type for the return type of `VecMap::move_iter` - Rename all type parameters to `V` for "Value". - Remove unnecessary call to an `Option::unwrap`, use pattern matching instead. - Remove incorrect `Hash` implementation which took the `VecMap`'s capacity into account. This is a [breaking-change], however whoever used the `Hash` implementation relied on an incorrect implementation.
| @@ -921,23 +919,6 @@ mod test_map { | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_hash() { | |||
There was a problem hiding this comment.
Uh... why delete the entire test?
There was a problem hiding this comment.
Oh you straight-up deleted the Hash impl. That seems wrong?
There was a problem hiding this comment.
You should be able to steal the Hash impl from e.g. RingBuf. Just hash the contents of the iterator.
Also re-add the previously deleted test with an additional test that would have failed before, when the hash function depended on the capacity.
| pub type Values<'a, T> = | ||
| iter::Map<'static, (uint, &'a T), &'a T, Entries<'a, T>>; | ||
| pub type Values<'a, V> = | ||
| iter::Map<'static, (uint, &'a V), &'a V, Entries<'a, V>>; |
There was a problem hiding this comment.
I'm vaguely concerned that these are typedefs instead of wrapper structs. Are we sure we're never going to want to change the implementations of these iterators such that this signature will change?
There was a problem hiding this comment.
Yes that's the more correct solution, longterm. If @tbu- is willing to make that change in this PR I would be happy to re-r.
bors
added a commit
that referenced
this pull request
Dec 10, 2014
- Introduce a named type for the return type of `VecMap::move_iter` - Rename all type parameters to `V` for "Value". - Remove unnecessary call to an `Option::unwrap`, use pattern matching instead. - Remove incorrect `Hash` implementation which took the `VecMap`'s capacity into account. This is a [breaking-change], however whoever used the `Hash` implementation relied on an incorrect implementation.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
VecMap::move_iterVfor "Value".Option::unwrap, use pattern matching instead.Hashimplementation which took theVecMap's capacityinto account.
This is a [breaking-change], however whoever used the
Hashimplementationrelied on an incorrect implementation.