-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add abomonation instance for Arc<T> #35
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @rklaehn! We should also add the instances for str
and [T]
. In our repo I only added [u8]
for fear of exhume
being called for each byte, but that may not actually be a problem (rustc might be smart enough).
* pointer to an ArcInner that we need to dispose of without trying to run | ||
* its destructor (which would panic). | ||
*/ | ||
let (value, bytes) = decode::<T>(bytes)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frankmcsherry Here value
contains a reference to a T
. Since T: 'static
this structure cannot contain non-static references (but it could contain boxes). My understanding is that T can only directly reference bytes
by unsafely reinterpreting a reference as a box or equivalently by transmuting a lifetime to 'static
. Is my assumption correct that such behaviour violates the contract of Abomonation?
*/ | ||
let (value, bytes) = decode::<T>(bytes)?; | ||
// value is just a reference to the first part of old bytes, so move it into a new Arc | ||
let arc = Arc::new(ptr::read(value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we lift the bytes from the underlying slice and move them into the ArcInner, i.e. we move the value of type T
to a new memory location. Since it cannot reference bytes
by the above argument, the result should be a correctly self-contained Arc<T>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be the problematic moment. Something like String
can still reference bytes
even after a ptr::read
. The read will only grab the 24 bytes of the String
type, but it may contain a pointer in to bytes
(and very likely does, as this is how String
implements Abomonation
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that’s evil! So the only correct implementation would be to require T: Clone
and clone the referenced value returned by decode
.
Hello! Sorry for the late review. The thing that worries me here is that afaict the For example, in your test you could imagine decoding the Does that make sense to you, or should I try and modify the test to demonstrate this? |
If this approach turns out to be correct, I guess we could do the same for Rc...