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

Using default and regular assignments doesn't guarantee overwriting everything #7

Closed
cesarb opened this issue Jan 18, 2017 · 2 comments

Comments

@cesarb
Copy link
Owner

cesarb commented Jan 18, 2017

As mentioned in #3, just assigning a Default::default() might not overwrite everything in case the value contains an enum (most probably an Option).

Simply using mem::zeroed is not a good idea if the type contains something for which an all-zero-bits value is invalid.

Possible solution: convert the &mut T to *mut T, and do a memset to zero immediately followed by a ptr::write of the Default::default(), which is valid regardless of dst's contents, since ptr::write ignores the target's contents (it's the official way to initialize a mem::uninitialized()), followed then by the hide_mem. The compiler should be able to elide the memset in the common case where just a Default is enough to overwrite everything.

cc #5, @burdges, @bluss

@burdges
Copy link
Contributor

burdges commented Jan 18, 2017

I'd imagine the reverted commit ccb563e#diff-ac56d72e9f4cbe36b5de6fb90bd33456R33 solves the problem because the ptr::write copies from Default::default() without dropping the value already there, so the only drop should be the *self = mem::zeroed::<Self>(); I'd imagine LLVM would optimizes it to write only once, but not sure.

burdges added a commit to burdges/clear_on_drop that referenced this issue Jan 18, 2017
This might address the concerns exlained in
cesarb#3 (comment) and
cesarb#7

This reverts commit 88b4c4f.

Conflicts:
	src/clearable.rs
@cesarb
Copy link
Owner Author

cesarb commented Jan 21, 2017

Fixed by the ptr::write_bytes in commit 517686e.

@cesarb cesarb closed this as completed Jan 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants