Make vec::IntoIter covariant again
#35733
Conversation
|
This uses integer indices instead of pointers to avoid aliasing struct IntoIter<T> {
buf: Shared<T>,
cap: usize,
head: Shared<T>,
tail: Shared<T>,
} |
| @@ -1812,27 +1786,22 @@ impl<T> DoubleEndedIterator for IntoIter<T> { | |||
| #[inline] | |||
| fn next_back(&mut self) -> Option<T> { | |||
| unsafe { | |||
| if self.end == self.ptr { | |||
| if self.head == self.tail { | |||
There was a problem hiding this comment.
This could also be: if self.is_empty(), I think?
|
In the past this has been pretty performance-sensitive, could you benchmark just a forward iteration of a Also nominating for backport as beta was cut today and we don't want to miss this. |
alexcrichton
added
beta-nominated
|
@alexcrichton Ah, if someone else could help out with benchmarks, that'd be great. If they indicate that this approach is slower, I'd be happy to rewrite this to use the other approach I mentioned. |
|
@apasel422 could you gist the IR for this: extern {
fn bar(a: u8);
}
pub fn sum(v: Vec<u8>) {
for e in v.into_iter() {
unsafe { bar(e); }
}
}For me, |
|
Hm so that's different because LLVM still has an add instruction, I'm not sure how much that'll end up translating to missed optimizations though. Maybe it's worth to stick to a two-pointers-approaching-each-other implementation though? |
|
@alexcrichton Done. |
Make `vec::IntoIter` covariant again Closes #35721 r? @alexcrichton
|
Might be worth backporting this to beta, as the regression lived just long enough to jump into beta. |
|
@alexcrichton Can we get this backported to beta? |
|
Yes this is tagged with beta-nominated, and we'll discuss at the next libs triage meeting. It'll likely be an obvious "yes let's backport" |
The regression in rust-lang/rust#35721 will most likely not be backported to beta until next week - see rust-lang/rust#35733 - so it probably makes sense to disable it for now.
|
Discussion at @rust-lang/libs today decided to accept for backport |
alexcrichton
added
the
beta-accepted
brson
removed
the
beta-nominated
Closes #35721
r? @alexcrichton