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

Simplify OwnedFace; remove usage of Pin #4

Closed
wants to merge 1 commit into from

Conversation

dhardy
Copy link

@dhardy dhardy commented Aug 12, 2020

As I understand it, the raison d'etre of Pin is to allow returning a type to the user which cannot be moved. Without a &mut reference or direct ownership of the (unboxed) value this isn't possible anyway. See here.

Further, we don't actually care about SelfRefVecFace being moved, we only care about the data (which is itself heap-allocated) being moved.

I left SelfRefVecFace boxed only because the face value is large enough that it probably shouldn't be copied around too much, but it's not required for safety.

This PR simplifies things significantly without compromising safety or breaking the API.

@dhardy
Copy link
Author

dhardy commented Aug 12, 2020

On further thoughts, it doesn't really make sense to box SelfRefVecFace then store this in an Arc (FontArc). So maybe we should just rename SelfRefVecFaceOwnedFace and stick #[inline] on the ctor? I'll go ahead if you agree, but it does mean if the result is not immediately stored inside a Box/Rc/Arc that some unnecessary copying will happen.

@lunabunn
Copy link

On further thoughts, it doesn't really make sense to box SelfRefVecFace then store this in an Arc (FontArc). So maybe we should just rename SelfRefVecFaceOwnedFace and stick #[inline] on the ctor? I'll go ahead if you agree, but it does mean if the result is not immediately stored inside a Box/Rc/Arc that some unnecessary copying will happen.

While I realize that this PR is currently stale, just for the record: this (removing the Box) would be unsound under both SB and TB. See #14.

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