rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation56 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

///
/// [`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")]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome and thanks for the PR !

☔ 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

r=me with a tracking issue

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?

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

Added tracking issue of #79995 to the attributes

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

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

⌛ Testing commit 5873f9f5b00401c50d723374922bb4511e86cc3b with merge b9e30d31972a1f45e6a7e3f6c412e2ac1e6abd61...

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

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.

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?

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

📌 Commit 4652a13 has been approved by 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 15, 2020

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request

Dec 15, 2020

…, 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.

This was referenced

Dec 16, 2020


drop(src);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. ^^

Labels

A-raw-pointers

Area: raw pointers, MaybeUninit, NonNull

merged-by-bors

This PR was explicitly merged by bors.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.