Uh oh!
There was an error while loading. Please reload this page.
Implement values_mut on {BTree, Hash}Map - #32633
Conversation
rust-highfive
commented
Mar 31, 2016
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. |
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.
frewsxcv
commented
Mar 31, 2016
Maybe I'll wait for #32635 merge before moving forward on this. |
frewsxcv
commented
Apr 1, 2016
#32635 merged and this PR is now rebased on top of those changes. |
alexcrichton
commented
Apr 1, 2016
Looks the travis error is legit (an error in one of the tests) |
frewsxcv
commented
Apr 1, 2016
@alexcrichton Which error are you looking at? If possible, could you paste in here? |
frewsxcv
commented
Apr 1, 2016
Nevermind. Found it. Fixing. |
alexcrichton
commented
Apr 1, 2016
Looks like the features in the doctests may still be off? (I'd recommend running |
frewsxcv
commented
Apr 2, 2016
Seems to pass the tests now. |
alexcrichton
commented
Apr 3, 2016
bors
commented
Apr 3, 2016
Implement `values_mut` on `{BTree, Hash}Map`
#32551
#32551