Added [T; N]::zip() #79451
Added [T; N]::zip() #79451
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (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. |
Welcome and thanks for the time you put into that ! |
|
@rustbot modify labels: T-libs |
| // unsafe { crate::mem::transmute::<[MaybeUninit<U>; N], [U; N]>(dst) } | ||
| // SAFETY: At this point we've properly initialized the whole array | ||
| // and we just need to cast it to the correct type. | ||
| unsafe { crate::mem::transmute_copy::<_, [(T, U); N]>(&dst) } |
There was a problem hiding this comment.
This implementation (x86-64 codegen with -O) uses more stack space and includes branches/a loop compared to this one (which is branch-free and utilizes less stack, at least for smaller sizes):
let mut dst = MaybeUninit::<[(T, U); N]>::uninit();
let ptr = dst.as_mut_ptr() as *mut (T, U);
for (idx, (lhs, rhs)) in IntoIter::new(lhs).zip(IntoIter::new(rhs)).enumerate() {
unsafe { ptr.add(idx).write((lhs, rhs)) }
}
unsafe { dst.assume_init() }https://godbolt.org/z/T4aT78
There was a problem hiding this comment.
However, that codegen is still not optimal (the memcpy is basically redundant and N/RVO could definitely help here).
There was a problem hiding this comment.
Are [MaybeUninit<(T, U)>; N] and [(T, U); N] layout compatible/equivalent? Otherwise that transmute_copy would be UB.
There was a problem hiding this comment.
I am quite new to working with unsafe but the doc states
[...] MaybeUninit will always guarantee that it has the same size, alignment, and ABI as T [...]
and there are examples with [Vec<u32>; N]) and [u8; N]. However I am not sure if there is anything special with [(T, U); N] that would make it not work.
There was a problem hiding this comment.
Looking at map() from #75212, which I based my function on, uses unsafe { crate::mem::transmute_copy::<_, [U; N]>(&dst) } for any U. That would make the same for [(T, U); N] (not the same U) fine, right?
There was a problem hiding this comment.
ah ty for the update! The asm just helps me see what differences there are between the two versions, mainly looking for bounds checks which shouldn't be necessary here. The version you have has a loop in the LBB1 block, whereas cynecx's has no loop and is unrolled as he mentions in his top comment. If you increase the size of the arrays, you'll start to see more differences between the two.
The one thing is, if you change
for ((lhs, rhs), dst) in IntoIter::new(lhs).zip(IntoIter::new(rhs)).zip(&mut dst) {
dst.write((lhs, rhs));
}
into
for (i, (lhs, rhs)) in IntoIter::new(lhs).zip(IntoIter::new(rhs)).enumerate() {
dst[i].write((lhs, rhs));
}
both versions output the same asm. So the main difference between the two is here if you choose to zip or enumerate, rather than whether you use a pointer or a MaybeUninit
There was a problem hiding this comment.
Then would that be the preferred implementation (zip3 on godbolt)? Seems to me like best of both worlds, no extra unsafe yet the same asm?
There was a problem hiding this comment.
To me that seems good, but I think that's ultimately up to you & the reviewer! I was just curious how each impl compared to the others.
There was a problem hiding this comment.
Do you all think zip3 is the way to go? Or do you have any other suggestions? :)
|
I just saw that there is a tracking issue for array methods #76118 |
|
Reassigning to a libs team member: This seems like a pretty niche feature to me personally, but I can understand that there are situations when you might want it. I'm not sure if the libs team has a standard for "fill in the missing methods" PRs like this. |
…fJung Constier maybe uninit I was playing around trying to make `[T; N]::zip()` in rust-lang#79451 be `const fn`. One of the things I bumped into was `MaybeUninit::assume_init`. Is there any reason for the intrinsic `assert_inhabited<T>()` and therefore `MaybeUninit::assume_init` not being `const`? --- I have as best as I could tried to follow the instruction in [library/core/src/intrinsics.rs](https://github.com/rust-lang/rust/blob/master/library/core/src/intrinsics.rs#L11). I have no idea what I am doing but it seems to compile after some slight changes after the copy paste. Is this anywhere near how this should be done? Also any ideas for name of the feature gate? I guess `const_maybe_assume_init` is quite misleading since I have added some more methods. Should I add test? If so what should be tested?
There was a problem hiding this comment.
This is my first PR to rust so I hope I have done everything right, or at least close :)
Welcome and thanks for working on this!
My implementation of zip() is mostly just a modified copy-paste of map(). Should I keep the comments?
Maybe there's a way to implement .zip() by using .map()?
Also am I right in assuming there should be no way for the for-loop to panic, thus no need for the dropguard seen in the map()-function?
Yes.
Can you open a tracking issue for this feature?
Co-authored-by: Mara Bos <[email protected]>
Thank you :) Did you have something like pub fn zip4<T, U, const N: usize>(lhs: [T; N], rhs: [U; N]) -> [(T, U); N] {
let mut rhs = IntoIter::new(rhs);
lhs.map(|lhs| (lhs, rhs.next().unwrap()))
}in mind(
Should this PR have its own tracking issue or would it make more sense to track it in "Tracking Issue for |
Yeah maybe. That particular example indeed doesn't produce the best code it seems. In that godbolt output, the panic message of
Yes, let's use a separate issue. Then if there is any discussion on this feature, it doesn't mix with other discussions about other functions. |
I suppose I am done experimenting now :) (unless someone wants something more tested) |
|
Thanks again! @bors r+ |
|
📌 Commit 8b37259 has been approved by |
|
⌛ Testing commit 8b37259 with merge 7abbce2bbe7949b0ca3c96c7afbe58a9bf6b639a... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
Is that my fault? Is there something I should do? :) |
|
Oh, that looks unrelated. Let's try again. @bors retry |
Added [T; N]::zip() This is my first PR to rust so I hope I have done everything right, or at least close :) --- This is PR adds the array method `[T; N]::zip()` which, in my mind, is a natural extension to rust-lang#75212. My implementation of `zip()` is mostly just a modified copy-paste of `map()`. Should I keep the comments? Also am I right in assuming there should be no way for the `for`-loop to panic, thus no need for the dropguard seen in the `map()`-function? The doc comment is in a similar way a slightly modified copy paste of [`Iterator::zip()`](https://doc.rust-lang.org/beta/std/iter/trait.Iterator.html#method.zip) `@jplatte` mentioned in [rust-lang#75490](rust-lang#75490 (comment)) `zip_with()`, > zip and zip_with seem like they would be useful :) is this something I should add (assuming there is interest for this PR at all :))
Added [T; N]::zip() This is my first PR to rust so I hope I have done everything right, or at least close :) --- This is PR adds the array method `[T; N]::zip()` which, in my mind, is a natural extension to rust-lang#75212. My implementation of `zip()` is mostly just a modified copy-paste of `map()`. Should I keep the comments? Also am I right in assuming there should be no way for the `for`-loop to panic, thus no need for the dropguard seen in the `map()`-function? The doc comment is in a similar way a slightly modified copy paste of [`Iterator::zip()`](https://doc.rust-lang.org/beta/std/iter/trait.Iterator.html#method.zip) ``@jplatte`` mentioned in [rust-lang#75490](rust-lang#75490 (comment)) `zip_with()`, > zip and zip_with seem like they would be useful :) is this something I should add (assuming there is interest for this PR at all :))
Added [T; N]::zip() This is my first PR to rust so I hope I have done everything right, or at least close :) --- This is PR adds the array method `[T; N]::zip()` which, in my mind, is a natural extension to rust-lang#75212. My implementation of `zip()` is mostly just a modified copy-paste of `map()`. Should I keep the comments? Also am I right in assuming there should be no way for the `for`-loop to panic, thus no need for the dropguard seen in the `map()`-function? The doc comment is in a similar way a slightly modified copy paste of [`Iterator::zip()`](https://doc.rust-lang.org/beta/std/iter/trait.Iterator.html#method.zip) ```@jplatte``` mentioned in [rust-lang#75490](rust-lang#75490 (comment)) `zip_with()`, > zip and zip_with seem like they would be useful :) is this something I should add (assuming there is interest for this PR at all :))
|
☀️ Test successful - checks-actions |
This is my first PR to rust so I hope I have done everything right, or at least close :)
This is PR adds the array method
[T; N]::zip()which, in my mind, is a natural extension to #75212.My implementation of
zip()is mostly just a modified copy-paste ofmap(). Should I keep the comments? Also am I right in assuming there should be no way for thefor-loop to panic, thus no need for the dropguard seen in themap()-function?The doc comment is in a similar way a slightly modified copy paste of
Iterator::zip()@jplatte mentioned in #75490
zip_with(),is this something I should add (assuming there is interest for this PR at all :))