Skip to content
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

Overload Iterator::last() for IntoIter. #76087

Closed
wants to merge 3 commits into from

Conversation

hbina
Copy link
Contributor

@hbina hbina commented Aug 29, 2020

Simply use the fact that IntoIter stores the end pointer and we
can perform pointer arithmetic.

NOTE: Where is the test for this?

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2020
@hbina hbina changed the title Overload last for Vec. Overload IntoIter::last() for Vec. Aug 29, 2020
@hbina hbina changed the title Overload IntoIter::last() for Vec. Overload Iterator::last() for Vec. Aug 29, 2020
@hbina hbina changed the title Overload Iterator::last() for Vec. Overload Iterator::last() for IntoIter. Aug 29, 2020
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Aug 30, 2020

This is unsound. The vec::IntoIter will be dropped when the function returns, in turn dropping every element including the one you just copied into the return place. unsafe should be used with care and only when there are no viable alternatives.

library/alloc/src/vec.rs Outdated Show resolved Hide resolved
library/alloc/src/vec.rs Outdated Show resolved Hide resolved
library/alloc/src/vec.rs Outdated Show resolved Hide resolved
@hbina
Copy link
Contributor Author

hbina commented Aug 30, 2020

I changed the implementation a bit copying what next() does except it marches to the end of the iterator immediately (which makes it similar to folding until the last element calling next() each time)

library/alloc/src/vec.rs Outdated Show resolved Hide resolved
library/alloc/src/vec.rs Outdated Show resolved Hide resolved
@ollie27
Copy link
Member

ollie27 commented Aug 30, 2020

This was attempted before in #57246 but as pointed out in #57246 (comment) this will change the drop order so may be considered a breaking change.

EDIT: looking at this closer the drop order should be the same so this should be fine.

@hbina
Copy link
Contributor Author

hbina commented Aug 30, 2020

I am quite new to raw Rust but AFAIK, but shouldn't the Drop order not be changed because the destructor is handled here by DropGuard

rust/library/alloc/src/vec.rs

Lines 2762 to 2781 in db534b3

#[stable(feature = "rust1", since = "1.0.0")]
unsafe impl<#[may_dangle] T> Drop for IntoIter<T> {
fn drop(&mut self) {
struct DropGuard<'a, T>(&'a mut IntoIter<T>);
impl<T> Drop for DropGuard<'_, T> {
fn drop(&mut self) {
// RawVec handles deallocation
let _ = unsafe { RawVec::from_raw_parts(self.0.buf.as_ptr(), self.0.cap) };
}
}
let guard = DropGuard(self);
// destroy the remaining elements
unsafe {
ptr::drop_in_place(guard.0.as_raw_mut_slice());
}
// now `guard` will be dropped and do the rest
}
}

I am not sure what this is for, tho
ptr::drop_in_place(guard.0.as_raw_mut_slice());

@ollie27
Copy link
Member

ollie27 commented Aug 30, 2020

Yeah, the Drop impl for IntoIter drops the remaining items front to back which is the same order as you'd get from repeatedly calling .next() so in this case it does match.

@bors
Copy link
Contributor

bors commented Sep 3, 2020

☔ The latest upstream changes (presumably #70793) made this pull request unmergeable. Please resolve the merge conflicts.

@jyn514 jyn514 added A-iterators Area: Iterators T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2020
@Dylan-DPC-zz
Copy link

@hbina any updates on this?

Simply use the fact that `IntoIter` stores the end pointer and we
can perform pointer arithmetic.
Simply does what next() does except it goes straight to the end of the
array.

Woopsie
@hbina hbina force-pushed the add_last_on_vec branch 2 times, most recently from 0307fd1 to 76d06f0 Compare October 23, 2020 06:52
@hbina
Copy link
Contributor Author

hbina commented Oct 23, 2020

@Dylan-DPC I have rebased to latest and accepted the change requested by @pickfire
Is there anything else you need me to do?

@ollie27
Copy link
Member

ollie27 commented Oct 23, 2020

As I mentioned in #76087 (comment) all last needs to do is call self.next_back().

Suggested by @pickfire.

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
@hbina hbina marked this pull request as ready for review October 23, 2020 15:26
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 9, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 9, 2020

NOTE: Where is the test for this?

Just testing for functionality can be done in library/alloc/tests/vec.rs. If you also want to test that .last() generates more efficient code than iterating over the entire range, a codegen test can be added in src/test/codegen, but that's probably not worth the effort.

@Dylan-DPC-zz
Copy link

r? @KodrAus

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 27, 2020

Does this make any difference in performance or binary size?

Both the previous and the proposed implementation end up iterating through all elements to Drop them.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2020
@bors
Copy link
Contributor

bors commented Dec 31, 2020

☔ The latest upstream changes (presumably #80530) made this pull request unmergeable. Please resolve the merge conflicts.

@hbina hbina closed this Dec 31, 2020
@hbina hbina deleted the add_last_on_vec branch December 25, 2022 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.