Implement From<Vec<T>> and Into<Vec<T>> for VecDeque<T>
#32866
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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. |
|
Ah, looks like I got the stability attribute wrong. Do I need to change the feature from rust1 to e.g. vecdeque_vec_conversions ? |
| impl<T> From<Vec<T>> for VecDeque<T> { | ||
| fn from(other: Vec<T>) -> Self { | ||
| unsafe { | ||
| let other_buf: *mut T = mem::transmute(other.as_ptr()); |
There was a problem hiding this comment.
If you declare other to be mut, this can just be:
let other_buf = other.as_mut_ptr();(The transmute is unnecessary.)
|
@davidhewitt These impls should probably start off unstable. You can use |
|
|
||
| // Do this to force resize to correct size; | ||
| // otherwise we may get not_power_of_two assert failures! | ||
| out.reserve(0); |
There was a problem hiding this comment.
It'd be good to test for the power-of-two invariant in your test for this method.
|
Thanks for the PR @davidhewitt! @apasel422 unfortunately we don't actually read stability annotations on trait impls right now, so these'll be insta-stable if they land (so It's also interesting implementing the |
alexcrichton
added
the
T-libs-api
|
@alexcrichton Hmm, I didn't even consider the |
|
Hi guys, I'm working on some corrections to this PR, might be a couple of evenings before they land if that's ok? R.E. the From / Into - shall I rework the Into into a From on the Vec class? |
|
@davidhewitt no worries! Feel free to ping this PR when it's updated. And yeah let's try to put |
|
@apasel422 @alexcrichton So I've taken the first round of comments on board and rewritten this patch with better tests, and covered some corner cases uncovered during testing. Thanks for the feedback, I feel I learned some good stuff with how to make better tests there! RE. putting the |
| for i in left_edge..right_edge { | ||
| right_offset = (i - left_edge) % (cap - right_edge); | ||
| let src: isize = (right_edge + right_offset) as isize; | ||
| println!("swap {} to {}; cap {}; left {}; len: {}; right {};", src, i, cap, left_edge, len, right_edge); |
There was a problem hiding this comment.
Looks like a stray println?
|
Thanks for the update @davidhewitt! The libs team discussed this in triage yesterday and the conclusion was that this is good-to-go (especially now that both impls are I'll leave the r+ to @apasel422 though :) @bors: delegate=apasel422 |
|
✌️ @apasel422 can now approve this pull request |
|
Thanks @alexcrichton! @apasel422 - I took "Closes #32848" out of the PR message, just checking that's what you wanted? |
| let cap = other.cap(); | ||
|
|
||
| // Need to move the ring to the front of the buffer, as vec will expect this. | ||
| if other.is_contiguous() { |
There was a problem hiding this comment.
Would it be possible to ensure that test_vec_from_vecdeque explicitly tests each of the four cases?
|
This looks good, pending some additional tests. Once you've done that, could you squash all the commits into one? |
davidhewitt
force-pushed
the
master
branch
2 times, most recently
from
95fc8e7 to
89f92cd
Compare
|
I rewrote test_vec_from_vecdeque to expand out the loops, to label which configurations of the loop variables will be testing which bits of the ring-straightening algorithm. Also squashed the commits. Is there other tests you have in mind? |
|
Looks like there are a few lines longer than 100 characters. Otherwise, this is ready to merge. |
|
@davidhewitt Looks like this failed with a syntax error. |
|
Sorry, made a mistake breaking up the lines. :/ |
|
@alexcrichton Is |
|
Ah nah we're on to 1.10.0 at this point. Should write a tidy check for that somehow... |
|
Ah okay - I'll fix that up tonight! |
Implement `From<Vec<T>>` and `Into<Vec<T>>` for `VecDeque<T>`
No description provided.