Implement values_mut on {BTree, Hash}Map
#32633
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| /// Mutable HashMap values iterator. | ||
| #[unstable(feature = "map_values_mut", reason = "recently added", issue = "32551")] | ||
| pub struct ValuesMut<'a, K: 'a, V: 'a> { | ||
| inner: Map<IterMut<'a, K, V>, fn((&'a K, &'a mut V)) -> &'a mut V> |
There was a problem hiding this comment.
Is there any reason that you chose to use Map here and not in BTreeMap's ValuesMut? I don't think that there are variance issues (since &mut forces V to be invariant anyway), but the inconsistency seems a little weird.
There was a problem hiding this comment.
Actually, now that I think about it some more, using Map makes K incorrectly invariant. This probably doesn't matter in practice, but I'd still be more comfortable implementing that mapping by hand and getting the correct covariance.
There was a problem hiding this comment.
I chose Map here for HashMap::ValuesMut because HashMap::Values uses a Map.
There was a problem hiding this comment.
Ah, that makes sense. I think the existing iterators are actually incorrect with respect to variance then - see #30642.
|
Maybe I'll wait for #32635 merge before moving forward on this. |
|
#32635 merged and this PR is now rebased on top of those changes. |
|
Looks the travis error is legit (an error in one of the tests) |
|
@alexcrichton Which error are you looking at? If possible, could you paste in here? |
|
Nevermind. Found it. Fixing. |
|
Looks like the features in the doctests may still be off? (I'd recommend running |
|
Seems to pass the tests now. |
Implement `values_mut` on `{BTree, Hash}Map`
#32551
#32551