Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename ManuallyDrop::take to read #62198

Open
wants to merge 2 commits into
base: master
from

Conversation

@CAD97
Copy link
Contributor

commented Jun 28, 2019

Tracking issue: #55422

Renames ManuallyDrop::take to match ptr::read and MaybeUninit::read, and updates the documentation to more mirror MaybeUninit::read's documentation.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2019

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@CAD97

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

@rust-highfive rust-highfive assigned SimonSapin and unassigned dtolnay Jun 28, 2019

@CAD97 CAD97 referenced this pull request Jun 28, 2019

Open

Tracking issue for `ManuallyDrop::take` #55422

1 of 3 tasks complete
@Centril

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

This is more consistent with MaybeUninit. I am not sure if that's what we want, but I proposed it, so I am curious what T-libs thinks. :)

@RalfJung RalfJung added the T-libs label Jun 28, 2019

@cramertj

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

It's surprising to me that a method called read would mutate the container, especially to the point of leaving its contents in an invalid state.

@CAD97

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

method called read would mutate the container

It doesn't actually mutate the container, just like ptr::read doesn't mutate what's behind the pointer. Rather, the state is changed logically (just like with ptr::read which logically moves the value out) without changing anything in memory.

I only used &mut slot.value because I already had &mut and forgot I didn't need it 😅. The only reason ManuallyDrop::read takes &mut currently rather than & like MaybeUninit::read is as a lint; it's almost certainly unsound to use if someone else has a reference as well. That said, MaybeUninit::read uses &self and ptr::read uses *const, so if the idea is to parallel those, this should probably use &slot as well.

The other option where we use take(&mut self) is treating ManuallyDrop as basically an untagged, unchecked Option with Deref sugar due to being Some 99.99% of its lifetime. The only time I expect this function will be used soundly is in drop(&mut self).

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

That said, MaybeUninit::read uses &self and ptr::read uses *const, so if the idea is to parallel those, this should probably use &slot as well.

Good point, now I wonder if MaybeUninit::read should take &mut self...

@CAD97

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Relevant note: this method has just been mentioned in a blog post as take.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

The link seems to be broken?

@CAD97

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

@RalfJung fixed (missing http:// messed up GitHub's URL parser)

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Relevant note: this method has just been mentioned in a blog post as take.

I'm sure @pnkfelix could change that if need be when we decide. :)

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

ping from triage @SimonSapin any updates on this?

@CAD97

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Per #55422 (comment), MaybeUninit::read may get renamed, removing that reasoning for the rename. Whether the function gets called read or take, the documentation updates in this PR I think are a clear improvement.

I'm happy to remove the rename if we can agree take is the better name.

@ProgrammaticNajel

This comment has been minimized.

Copy link

commented Aug 2, 2019

Ping from triage. @SimonSapin any updates on this? Thanks.

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

can anyone from @rust-lang/libs review this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.