MaybeUninit::copy/clone_from_slice #79607

beepster4096 · 2020-12-01T20:26:44Z

This PR adds 2 new methods to MaybeUninit under the feature of maybe_uninit_write_slice: copy_from_slice and clone_from_slice.

These are useful for initializing uninitialized buffers (such as the one returned by Vec::spare_capacity_mut for example) with initialized data.

The methods behave similarly to the methods on slices, but the destination is uninitialized and they return the destination slice as an initialized slice.

rust-highfive · 2020-12-01T20:26:47Z

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.

m-ou-se

Thanks!

Looks like a useful addition, especially in combination with Vec::spare_capacity_mut.

I'm wondering if the names of these functions should be different. The new copy_from_slice is mostly equivalent to [T]::copy_from_slice, but [T]::clone_from_slice drops old values whereas this new one doesn't. Maybe it should use write in the name, to make clear it behaves like MaybeUninit::write or ptr::write. What do you think?

library/core/src/mem/maybe_uninit.rs

m-ou-se · 2020-12-01T20:48:05Z

library/core/src/mem/maybe_uninit.rs

+    ///
+    /// [`clone_from_slice`]: MaybeUninit::clone_from_slice
+    /// [`slice::copy_from_slice`]: ../../std/primitive.slice.html#method.copy_from_slice
+    #[unstable(feature = "maybe_uninit_write_slice", issue = "none")]


Feel free to open a tracking issue, and then add its number here.

beepster4096 · 2020-12-01T21:11:52Z

I'm wondering if the names of these functions should be different. The new copy_from_slice is mostly equivalent to [T]::copy_from_slice, but [T]::clone_from_slice drops old values whereas this new one doesn't. Maybe it should use write in the name, to make clear it behaves like MaybeUninit::write or ptr::write. What do you think?

Maybe write_slice and write_slice_cloned?

poliorcetics

Welcome and thanks for the PR !

library/core/src/mem/maybe_uninit.rs

cramertj · 2020-12-08T23:06:20Z

r? @sfackler

library/core/src/mem/maybe_uninit.rs

bors · 2020-12-10T13:17:11Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

sfackler · 2020-12-10T23:46:31Z

r=me with a tracking issue

m-ou-se · 2020-12-11T00:22:56Z

Maybe write_slice and write_slice_cloned?

Sounds good. That should make it a bit clearer that they only write, and don't drop old elements like []::clone_from_slice does.

Can you squash the commits?

beepster4096 · 2020-12-12T03:49:39Z

Squashed and renamed.

m-ou-se · 2020-12-12T09:00:25Z

Can you open a tracking issue for this, and put its number in the #[unstable] attributes? Then we can merge this. :)

beepster4096 · 2020-12-13T02:23:21Z

Added tracking issue of #79995 to the attributes

m-ou-se · 2020-12-14T13:11:37Z

Thanks!

@bors r+

bors · 2020-12-14T13:11:39Z

📌 Commit 5873f9f5b00401c50d723374922bb4511e86cc3b has been approved by m-ou-se

bors · 2020-12-14T19:08:00Z

⌛ Testing commit 5873f9f5b00401c50d723374922bb4511e86cc3b with merge b9e30d31972a1f45e6a7e3f6c412e2ac1e6abd61...

bors · 2020-12-14T19:34:41Z

💔 Test failed - checks-actions

m-ou-se · 2020-12-14T19:41:46Z

The uninit_write_slice_cloned_mid_panic test failed on wasm, probably because unwinding is not available and panic::catch_unwind doesn't catch the panic.

beepster4096 · 2020-12-14T20:11:22Z

The uninit_write_slice_cloned_mid_panic test failed on wasm, probably because unwinding is not available and panic::catch_unwind doesn't catch the panic.

Do panics abort on wasm? Would #[cfg(panic = "unwind")] on the test fix it?

m-ou-se · 2020-12-15T20:00:01Z

Yes. Looks like other tests are using that same cfg too, so that should be fine. (Some tests just explicitly ignore the wasm target for this reason, but those were written before cfg(panic = ..) existed.)

m-ou-se · 2020-12-15T20:33:59Z

@bors r+

bors · 2020-12-15T20:34:01Z

📌 Commit 4652a13 has been approved by m-ou-se

…, r=m-ou-se MaybeUninit::copy/clone_from_slice This PR adds 2 new methods to MaybeUninit under the feature of `maybe_uninit_write_slice`: `copy_from_slice` and `clone_from_slice`. These are useful for initializing uninitialized buffers (such as the one returned by `Vec::spare_capacity_mut` for example) with initialized data. The methods behave similarly to the methods on slices, but the destination is uninitialized and they return the destination slice as an initialized slice.

bors · 2020-12-16T06:26:57Z

⌛ Testing commit 4652a13 with merge ddbc617...

bors · 2020-12-16T09:13:08Z

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

RalfJung · 2020-12-17T11:46:30Z

library/core/tests/mem.rs

+
+    drop(src);
+
+    assert_eq!(Rc::strong_count(&rc), 2);


I think this test made Miri's run on the test suite fail, because it introduced a memory leak... can the test be adjusted to not leak memory?

If the goal is just to test that Drop is not called, the better way is to use a type that panics on drop.

I confirmed that this function is the culprit, so I opened an issue: #80116

@RalfJung Thanks for debugging this! I'll watch for leaks in tests more closely in future reviews. :)

@drmeepster Do you feel like changing it to a panic-on-drop to avoid the leak? Otherwise I can also make some time for this. ^^

rust-highfive assigned cramertj Dec 1, 2020

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

m-ou-se added A-raw-pointers Area: raw pointers, MaybeUninit, NonNull T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 1, 2020

m-ou-se assigned m-ou-se and unassigned cramertj Dec 1, 2020

m-ou-se reviewed Dec 1, 2020

View reviewed changes

poliorcetics reviewed Dec 1, 2020

View reviewed changes

cynecx reviewed Dec 2, 2020

View reviewed changes

library/core/src/mem/maybe_uninit.rs Outdated Show resolved Hide resolved

beepster4096 mentioned this pull request Dec 5, 2020

Tracking Issue for RFC 2930 (read-buf) #78485

Open

5 tasks

gThorondorsen reviewed Dec 5, 2020

View reviewed changes

library/core/src/mem/maybe_uninit.rs Outdated Show resolved Hide resolved

rust-highfive assigned sfackler and unassigned m-ou-se Dec 8, 2020

sfackler reviewed Dec 9, 2020

View reviewed changes

library/core/src/mem/maybe_uninit.rs Outdated Show resolved Hide resolved

beepster4096 force-pushed the maybe_uninit_write_slice branch from 74dbaec to 125ca5b Compare December 10, 2020 20:42

beepster4096 force-pushed the maybe_uninit_write_slice branch 2 times, most recently from 7024d7f to 87522c4 Compare December 12, 2020 03:44

beepster4096 mentioned this pull request Dec 13, 2020

Tracking Issue for maybe_uninit_write_slice #79995

Open

4 tasks

beepster4096 force-pushed the maybe_uninit_write_slice branch from 122dae8 to 5873f9f Compare December 13, 2020 02:14

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 14, 2020

m-ou-se assigned m-ou-se and unassigned sfackler Dec 14, 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 14, 2020

write_slice(_cloned)

4652a13

beepster4096 force-pushed the maybe_uninit_write_slice branch from 5873f9f to 4652a13 Compare December 15, 2020 20:23

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 15, 2020

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

Rollup of 11 pull requests #80064

Closed

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

bors merged commit ddbc617 into rust-lang:master Dec 16, 2020

rustbot added this to the 1.50.0 milestone Dec 16, 2020

This was referenced Dec 16, 2020

Add in-place optimization for array map #75571

Closed

Stop generating code in mem::forget #79989

Closed

RalfJung reviewed Dec 17, 2020

View reviewed changes

RalfJung mentioned this pull request Dec 17, 2020

Test "mem::uninit_write_slice_cloned_no_drop" leaks memory #80116

Closed

mgeier mentioned this pull request Dec 26, 2020

MaybeUninit::write_slice_cloned() makes it very easy to accidentally leak #80376

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MaybeUninit::copy/clone_from_slice #79607

MaybeUninit::copy/clone_from_slice #79607

beepster4096 commented Dec 1, 2020

rust-highfive commented Dec 1, 2020

m-ou-se left a comment

m-ou-se Dec 1, 2020

beepster4096 commented Dec 1, 2020

poliorcetics left a comment

cramertj commented Dec 8, 2020

bors commented Dec 10, 2020

sfackler commented Dec 10, 2020

m-ou-se commented Dec 11, 2020

beepster4096 commented Dec 12, 2020

m-ou-se commented Dec 12, 2020

beepster4096 commented Dec 13, 2020

m-ou-se commented Dec 14, 2020

bors commented Dec 14, 2020

bors commented Dec 14, 2020

bors commented Dec 14, 2020

m-ou-se commented Dec 14, 2020

beepster4096 commented Dec 14, 2020

m-ou-se commented Dec 15, 2020

m-ou-se commented Dec 15, 2020

bors commented Dec 15, 2020

bors commented Dec 16, 2020

bors commented Dec 16, 2020

RalfJung Dec 17, 2020

RalfJung Dec 17, 2020

RalfJung Dec 17, 2020

m-ou-se Dec 17, 2020

MaybeUninit::copy/clone_from_slice #79607

MaybeUninit::copy/clone_from_slice #79607

Conversation

beepster4096 commented Dec 1, 2020

rust-highfive commented Dec 1, 2020

m-ou-se left a comment

Choose a reason for hiding this comment

m-ou-se Dec 1, 2020

Choose a reason for hiding this comment

beepster4096 commented Dec 1, 2020

poliorcetics left a comment

Choose a reason for hiding this comment

cramertj commented Dec 8, 2020

bors commented Dec 10, 2020

sfackler commented Dec 10, 2020

m-ou-se commented Dec 11, 2020

beepster4096 commented Dec 12, 2020

m-ou-se commented Dec 12, 2020

beepster4096 commented Dec 13, 2020

m-ou-se commented Dec 14, 2020

bors commented Dec 14, 2020

bors commented Dec 14, 2020

bors commented Dec 14, 2020

m-ou-se commented Dec 14, 2020

beepster4096 commented Dec 14, 2020

m-ou-se commented Dec 15, 2020

m-ou-se commented Dec 15, 2020

bors commented Dec 15, 2020

bors commented Dec 16, 2020

bors commented Dec 16, 2020

RalfJung Dec 17, 2020

Choose a reason for hiding this comment

RalfJung Dec 17, 2020

Choose a reason for hiding this comment

RalfJung Dec 17, 2020

Choose a reason for hiding this comment

m-ou-se Dec 17, 2020

Choose a reason for hiding this comment