Added [T; N]::zip() #79451

usbalbin · 2020-11-26T21:54:10Z

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 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()

@jplatte mentioned in #75490 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 :))

rust-highfive · 2020-11-26T21:54:13Z

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.

library/core/tests/array.rs

library/core/tests/lib.rs

poliorcetics · 2020-11-26T23:52:19Z

This is my first PR to rust

Welcome and thanks for the time you put into that !

poliorcetics · 2020-11-26T23:55:35Z

@rustbot modify labels: T-libs

cynecx · 2020-11-27T01:07:24Z

library/core/src/array/mod.rs

+        // 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) }


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

However, that codegen is still not optimal (the memcpy is basically redundant and ~~N/RVO could definitely help here~~).

Are [MaybeUninit<(T, U)>; N] and [(T, U); N] layout compatible/equivalent? Otherwise that transmute_copy would be UB.

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.

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?

Sorry for the delay. I added a zippy2 to @cynecx's godbolt link here. However I am not too familiar with judging asm quality :)

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

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?

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.

Do you all think zip3 is the way to go? Or do you have any other suggestions? :)

library/core/src/array/mod.rs

usbalbin · 2020-11-29T22:58:46Z

I just saw that there is a tracking issue for array methods #76118

cramertj · 2020-12-08T17:44:19Z

Reassigning to a libs team member:
r? @BurntSushi

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?

BurntSushi · 2020-12-14T13:21:19Z

@cramertj Ah I'm not in the review rotation. r? @m-ou-se

m-ou-se

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?

library/core/src/array/mod.rs

Co-authored-by: Mara Bos <m-ou.se@m-ou.se>

usbalbin · 2020-12-16T20:29:36Z

@m-ou-se

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()?

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(zip4 godbolt)? As said earlier, I am not that confident in my skills of judging asm, but if I am reading it right zip4 does not get unrolled and contains more jumps, right? Do you feel that the simpler implementation outweighs the (maybe/probably) worse asm?

Can you open a tracking issue for this feature?

Should this PR have its own tracking issue or would it make more sense to track it in "Tracking Issue for array_methods" - #76118?

m-ou-se · 2020-12-16T20:33:52Z

Did you have something like [..] in mind?

Yeah maybe. That particular example indeed doesn't produce the best code it seems. In that godbolt output, the panic message of .unwrap() is still there, which is not great. But maybe there's some other way to avoid that. It'd avoid having to use tricky unsafe code. But if it results in worse code, that's not worth it.

Should this PR have its own tracking issue [..] ?

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.

usbalbin · 2020-12-18T23:38:56Z

That looks perfect, thanks! If you add the tracking issue number in the #[unstable] attribute, we can merge this. (Unless you first want to experiment more with different implementations of the function, which is also fine.)

I suppose I am done experimenting now :) (unless someone wants something more tested)

m-ou-se · 2020-12-20T11:14:24Z

Thanks again!

@bors r+

bors · 2020-12-20T11:14:25Z

📌 Commit 8b37259 has been approved by m-ou-se

bors · 2020-12-20T19:41:11Z

⌛ Testing commit 8b37259 with merge 7abbce2bbe7949b0ca3c96c7afbe58a9bf6b639a...

rust-log-analyzer · 2020-12-20T19:52:31Z

The job x86_64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

      Memory: 14 GB
      Boot ROM Version: VMW71.00V.13989454.B64.1906190538
      Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
      SMC Version (system): 2.8f0
      Serial Number (system): VMiWaqwTYYq7

hw.ncpu: 3
hw.byteorder: 1234
hw.memsize: 15032385536
---
[RUSTC-TIMING] build_script_build test:false 0.473
[RUSTC-TIMING] build_script_build test:false 0.578
The following warnings were emitted during compilation:

warning: ar: no archive members specified
warning: usage:  ar -d [-TLsv] archive file ...
warning:  ar -m [-TLsv] archive file ...
warning:  ar -m [-abiTLsv] position archive file ...
warning:  ar -p [-TLsv] archive [file ...]
warning:  ar -q [-cTLsv] archive file ...
warning:  ar -r [-cuTLsv] archive file ...
warning:  ar -r [-abciuTLsv] position archive file ...
warning:  ar -t [-TLsv] archive [file ...]
warning:  ar -x [-ouTLsv] archive [file ...]
error: failed to run custom build command for `profiler_builtins v0.0.0 (/Users/runner/work/rust/rust/library/profiler_builtins)`

Caused by:
Caused by:
  process didn't exit successfully: `/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage0-std/release/build/profiler_builtins-1ff4284c8fc0d194/build-script-build` (exit code: 1)
  --- stdout
  TARGET = Some("x86_64-apple-darwin")
  HOST = Some("x86_64-apple-darwin")
  AR_x86_64-apple-darwin = Some("ar")
  running: "ar" "crs" "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage0-std/x86_64-apple-darwin/release/build/profiler_builtins-5ec280d6e6f2145b/out/libprofiler-rt.a"
  cargo:warning=ar: no archive members specified
  cargo:warning=usage:  ar -d [-TLsv] archive file ...
  cargo:warning= ar -m [-TLsv] archive file ...
  cargo:warning= ar -m [-abiTLsv] position archive file ...
  cargo:warning= ar -p [-TLsv] archive [file ...]
  cargo:warning= ar -q [-cTLsv] archive file ...
  cargo:warning= ar -r [-cuTLsv] archive file ...
  cargo:warning= ar -r [-abciuTLsv] position archive file ...
  cargo:warning= ar -t [-TLsv] archive [file ...]
  cargo:warning= ar -x [-ouTLsv] archive [file ...]

  --- stderr



  error occurred: Command "ar" "crs" "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage0-std/x86_64-apple-darwin/release/build/profiler_builtins-5ec280d6e6f2145b/out/libprofiler-rt.a" with args "ar" did not execute successfully (status code exit code: 1).

warning: build failed, waiting for other jobs to finish...
[RUSTC-TIMING] core test:false 68.505
error: build failed

bors · 2020-12-20T19:54:07Z

💔 Test failed - checks-actions

usbalbin · 2020-12-21T20:03:59Z

Test failed - checks-actions

Is that my fault? Is there something I should do? :)

m-ou-se · 2020-12-21T20:08:47Z

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 :))

bors · 2020-12-22T13:19:46Z

⌛ Testing commit 8b37259 with merge 0fe1dc6...

bors · 2020-12-22T16:09:52Z

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 0fe1dc6 to master...

rust-highfive assigned cramertj Nov 26, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2020

Added [T; N]::zip()

3b8617b

usbalbin force-pushed the array_zip branch from 636cbf9 to 3b8617b Compare November 26, 2020 22:23

poliorcetics reviewed Nov 26, 2020

View reviewed changes

library/core/tests/array.rs Outdated Show resolved Hide resolved

library/core/tests/lib.rs Outdated Show resolved Hide resolved

rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 26, 2020

cynecx reviewed Nov 27, 2020

View reviewed changes

Remove redundant tests

2f35fb1

usbalbin commented Nov 27, 2020

View reviewed changes

library/core/src/array/mod.rs Show resolved Hide resolved

usbalbin mentioned this pull request Nov 29, 2020

Tracking Issue for array_methods #76118

Closed

5 tasks

usbalbin mentioned this pull request Dec 2, 2020

Constier maybe uninit #79621

Merged

scottmcm mentioned this pull request Dec 4, 2020

Add Iterator::collect_array method #79659

Closed

cynecx mentioned this pull request Dec 5, 2020

Suboptimal codegen for potential [T; N]::zip() #79754

Closed

rust-highfive assigned BurntSushi and unassigned cramertj Dec 8, 2020

rust-highfive assigned m-ou-se and unassigned BurntSushi Dec 14, 2020

Update zip for better codegen, see discussion

be2c8f2

m-ou-se reviewed Dec 16, 2020

View reviewed changes

library/core/src/array/mod.rs Outdated Show resolved Hide resolved

Update doc comment

baa5e47

Co-authored-by: Mara Bos <m-ou.se@m-ou.se>

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 20, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2020

Dylan-DPC-zz mentioned this pull request Dec 21, 2020

Rollup of 15 pull requests #80276

Closed

Dylan-DPC-zz mentioned this pull request Dec 22, 2020

Rollup of 10 pull requests #80281

Closed

Dylan-DPC-zz mentioned this pull request Dec 22, 2020

Rollup of 9 pull requests #80282

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 22, 2020

bors merged commit 0fe1dc6 into rust-lang:master Dec 22, 2020

rustbot added this to the 1.50.0 milestone Dec 22, 2020

bors mentioned this pull request Dec 22, 2020

array::try_map #79713

Closed

dtolnay mentioned this pull request Jan 11, 2021

Add [T; N]::each_ref and [T; N]::each_mut #75490

Merged

lcnr added the A-const-generics Area: const generics (parameters and arguments) label Dec 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added [T; N]::zip() #79451

Added [T; N]::zip() #79451

usbalbin commented Nov 26, 2020 •

edited

Loading

rust-highfive commented Nov 26, 2020

poliorcetics commented Nov 26, 2020

poliorcetics commented Nov 26, 2020

cynecx Nov 27, 2020 •

edited

Loading

cynecx Nov 27, 2020 •

edited

Loading

cynecx Nov 27, 2020 •

edited

Loading

usbalbin Nov 27, 2020

usbalbin Nov 27, 2020

usbalbin Dec 12, 2020

JulianKnodt Dec 14, 2020 •

edited

Loading

usbalbin Dec 15, 2020

JulianKnodt Dec 15, 2020 •

edited

Loading

usbalbin Dec 16, 2020

usbalbin commented Nov 29, 2020

cramertj commented Dec 8, 2020

BurntSushi commented Dec 14, 2020

m-ou-se left a comment

usbalbin commented Dec 16, 2020 •

edited

Loading

m-ou-se commented Dec 16, 2020

usbalbin commented Dec 18, 2020 •

edited

Loading

m-ou-se commented Dec 20, 2020

bors commented Dec 20, 2020

bors commented Dec 20, 2020

rust-log-analyzer commented Dec 20, 2020

bors commented Dec 20, 2020

usbalbin commented Dec 21, 2020

m-ou-se commented Dec 21, 2020

bors commented Dec 22, 2020

bors commented Dec 22, 2020

Added [T; N]::zip() #79451

Added [T; N]::zip() #79451

Conversation

usbalbin commented Nov 26, 2020 • edited Loading

rust-highfive commented Nov 26, 2020

poliorcetics commented Nov 26, 2020

poliorcetics commented Nov 26, 2020

cynecx Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

cynecx Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

cynecx Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

usbalbin Nov 27, 2020

Choose a reason for hiding this comment

usbalbin Nov 27, 2020

Choose a reason for hiding this comment

usbalbin Dec 12, 2020

Choose a reason for hiding this comment

JulianKnodt Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

usbalbin Dec 15, 2020

Choose a reason for hiding this comment

JulianKnodt Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

usbalbin Dec 16, 2020

Choose a reason for hiding this comment

usbalbin commented Nov 29, 2020

cramertj commented Dec 8, 2020

BurntSushi commented Dec 14, 2020

m-ou-se left a comment

Choose a reason for hiding this comment

usbalbin commented Dec 16, 2020 • edited Loading

m-ou-se commented Dec 16, 2020

usbalbin commented Dec 18, 2020 • edited Loading

m-ou-se commented Dec 20, 2020

bors commented Dec 20, 2020

bors commented Dec 20, 2020

rust-log-analyzer commented Dec 20, 2020

bors commented Dec 20, 2020

usbalbin commented Dec 21, 2020

m-ou-se commented Dec 21, 2020

bors commented Dec 22, 2020

bors commented Dec 22, 2020

usbalbin commented Nov 26, 2020 •

edited

Loading

cynecx Nov 27, 2020 •

edited

Loading

cynecx Nov 27, 2020 •

edited

Loading

cynecx Nov 27, 2020 •

edited

Loading

JulianKnodt Dec 14, 2020 •

edited

Loading

JulianKnodt Dec 15, 2020 •

edited

Loading

usbalbin commented Dec 16, 2020 •

edited

Loading

usbalbin commented Dec 18, 2020 •

edited

Loading