-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Implement RFC 839 #25989
Implement RFC 839 #25989
Conversation
@@ -760,3 +760,10 @@ impl<T: Ord> Extend<T> for BinaryHeap<T> { | |||
} | |||
} | |||
} | |||
|
|||
#[unstable(feature = "extend_ref", reason = "recently added")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the way stability works these instability annotations unfortunately won't be read and considered. In this case that's ok, though, but could you change these to #[stable(feature = "extend_ref", since = "1.2.0")]
?
🎊 r=me with @alexcrichton's nits addressed |
Sorry for the delay! I modified the stability markers and removed |
@bors r+ |
📌 Commit e2dd771 has been approved by |
⌛ Testing commit e2dd771 with merge 0246d8d... |
💔 Test failed - auto-linux-64-x-android-t |
|
Closes rust-lang#25976.
Oh, sorry, I fixed it. |
@bors r+ No worries! |
📌 Commit b36ed7d has been approved by |
I had to use `impl<'a, V: Copy> Extend<(usize, &'a V)> for VecMap<V>` instead of `impl<'a, V: Copy> Extend<(&'a usize, &'a V)> for VecMap<V>` as that's what is needed for doing ```rust let mut a = VecMap::new(); let b = VecMap::new(); b.insert(1, "foo"); a.extend(&b) ``` I can squash the commits after review. r? @gankro
Such inference failures are interesting & important to note since this will be a minor breaking change in that case. (Which is permitted). Thanks for implementing this!! |
@brson Do we need to track this? This caused a very minor change to how Extend-using functions infer. In this case seen when using |
@bluss If it's a regression, yeah, I suppose so. Also nominating for beta for the same reason. |
@bluss I didn't spell out that in the RFC because I guess it seemed obvious: every generalization of an API can break someone's code in inference. |
We discussed this in triage today and decided to not backport this, we're going to have minor regressions like this and we're probably not going to aggressively backport them, they'll make their way into the stable channel eventually. |
It appears that these impls were left out of #25989 by mistake. r? @alexcrichton I'm not sure what the stability markers for these should be.
I had to use
impl<'a, V: Copy> Extend<(usize, &'a V)> for VecMap<V>
instead ofimpl<'a, V: Copy> Extend<(&'a usize, &'a V)> for VecMap<V>
as that's what is needed for doingI can squash the commits after review.
r? @gankro