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

Introduce a Clearable trait to support unsized types #5

Closed
wants to merge 8 commits into from

Conversation

burdges
Copy link
Contributor

@burdges burdges commented Jan 16, 2017

Introduce a Clearable trait to support unsized types. Addresses the most concerns in #3 as well, including the warning by @bluss that assignment might not zero everything #3 (comment)

I decided to implement Clearable for all Sized+Copy types because Shared<T> is the only dangerous Copy type I could find. there are many pointer types that satisfy Default but Shared<T> does not, so we can avoid all current pointer types if we used impl Clearable T where T: Copy+Default instead.

I added this in ccb563e but reverted it in 88b4c4f because Shared<T> does not seem so dangerous right now. I'd hope Rust gets type-level numerics before any new dangerous Shared<T> abstractions and any such abstractions are likely to satisfy Default anyways.

We do drop a zeroed object as a result of reverting the Default bounds, but afaik the worst case would be dropping a zeroed Shared<T>, whish does nothing itself. If anything worse happens, then you can revert the revert 88b4c4f of course. :)

Adds support for unsized types and provides a possible answer to
cesarb#3  However, the
`impl<T> Clearable for T where T: Copy` remains problematic because
`Shared<T>: Copy`.  There are worse implementers for `Default` like
`Box`, `Arc`, and `Rc`, but we could avoids all current pointer
types with `impl<T> Clearable for T where T: Copy+Default`.
I feel supporting fixed length arrays is more important right now.
Rust might have type level numerics and Default for fixed lenght
arrays before it gets a garbage collector or other dangerous types
based on `Shared<T>`.

This reverts commit ccb563e.
@burdges
Copy link
Contributor Author

burdges commented Jan 16, 2017

We should probably add some tests for unsized types besides the [T] in a Vec<T>, like maybe the [T] in a Box<[T]>.

Also move Drop closer to defition for readability and so that
AsRef and AsMut are closer to Deref and DerefMut.

Remove spurious use in test
@burdges
Copy link
Contributor Author

burdges commented Jan 16, 2017

Added impls for AsRef and AsMut. I'll send a separate pull request with my thoughts on Borrow and BorrowMut.

We can use a `ClearOnDrop` as the key for a `HashMap` with this change.
@burdges
Copy link
Contributor Author

burdges commented Jan 17, 2017

I should likely make this AsRef and Borrow stuff a separate pull request, as it's maybe a distraction here.

Just fyi, I tried two tricks to make ClearOnDrop behave kinda like it took a Borrow, but both failed : https://github.com/burdges/clear_on_drop/tree/borrow_fail/src https://github.com/burdges/clear_on_drop/tree/owned_fail/src

This limits how one can implement Clerable, but avoids the T: ?Sized
bounds added by e53f1b2 to hide_mem
which might not work. See:
cesarb#6 and
http://llvm.org/docs/LangRef.html#indirect-inputs-and-outputs

Also generalizes impl Clearable for [T] from T: Copy to T: Clearable
This might address the concerns exlained in
cesarb#3 (comment) and
cesarb#7

This reverts commit 88b4c4f.

Conflicts:
	src/clearable.rs
@burdges
Copy link
Contributor Author

burdges commented Jan 18, 2017

Just changed how unsized arrays are handled to hopefully deal with #6 (comment) I have not tried to verify that hide_mem really does not work on unsized types. Also I have not yet removed the ?Sized bounds this pull adds in hide_mem.rs, which probably should happen.

I maybe delt with #7 (comment) too, which required adding Default back.

@burdges
Copy link
Contributor Author

burdges commented Jan 19, 2017

I'm not confident what should be marked unsafe in the Clearable trait. In particular, there is less need for clear() to be unsafe now that it no longer leaves T in an unusable state. I think marking the Clearable trait itself as unsafe now more important since you cannot implement it yourself without using something like hide_mem.

Also ClearInPlace would be another possible name for Clearable. There is no need for a flashy name if almost nobody will be implement it themselves or use it as a constraint. :)

@cesarb
Copy link
Owner

cesarb commented Jan 21, 2017

I went in a slightly different direction (inspired by this pull request) with commit 517686e, and now allow [T] with the possibility of the user adding his own DSTs by implementing the InitializableFromZeroed trait.

With that change, this pull request will become hopelessly tangled, so I'll close it now; please open a new one based on the current master if there are still things to change. I'll probably release 0.2 in a few days.

@cesarb cesarb closed this 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

Successfully merging this pull request may close these issues.

2 participants