Remove sized requirement from Arc::from_raw #85
Conversation
stepancheg
force-pushed
the
arc-from-raw-unsized
branch
from
549eb8c
to
28ba066
Compare
src/arc.rs
Outdated
| @@ -39,6 +39,18 @@ pub(crate) struct ArcInner<T: ?Sized> { | |||
| unsafe impl<T: ?Sized + Sync + Send> Send for ArcInner<T> {} | |||
| unsafe impl<T: ?Sized + Sync + Send> Sync for ArcInner<T> {} | |||
|
|
|||
| impl<T: ?Sized> ArcInner<T> { | |||
| /// Safety: value must be a valid initialized pointer. | |||
There was a problem hiding this comment.
nit: please document what this function does, and also document that it is a safety-usable invariant
src/arc.rs
Outdated
| let offset_of_data = ArcInner::<T>::offset_of_data(ptr); | ||
|
|
||
| let ptr = ptr.byte_sub(offset_of_data); | ||
| Arc::from_raw_inner(ptr as *mut ArcInner<T>) |
There was a problem hiding this comment.
nit: please document why this usage is safe
stepancheg
force-pushed
the
arc-from-raw-unsized
branch
2 times, most recently
from
1799e98
to
6f0671c
Compare
|
Amended the PR with more comments. Not sure I made it more helpful then before. |
| // to subtract the offset of the `data` field from the pointer. | ||
|
|
||
| // SAFETY: `ptr` comes from `Arc`, so it must be initialized. | ||
| let offset_of_data = ArcInner::<T>::offset_of_data(ptr); |
There was a problem hiding this comment.
I think I'd prefer an explicit documentation of the fact that from_raw_inner wants the beginning of the allocation, and ptr points to the data.
src/arc.rs
Outdated
| // and resulting pointer is within the allocation. | ||
| let arc_inner_ptr = ptr.byte_sub(offset_of_data); | ||
|
|
||
| // SAFETY: `Arc::from_raw` contract requires that `ptr` comes from `Arc::into_raw`, |
There was a problem hiding this comment.
but this pointer doesn't come from into_raw? It's subtracted
stepancheg
force-pushed
the
arc-from-raw-unsized
branch
from
6f0671c
to
cb8f9f2
Compare
|
Rephrased. Hope it's better. |
stepancheg
force-pushed
the
arc-from-raw-unsized
branch
from
cb8f9f2
to
4a8cbe7
Compare
src/arc.rs
Outdated
| let offset_of_data = ArcInner::<T>::offset_of_data(ptr); | ||
|
|
||
| // SAFETY: `ptr` points to `ArcInner.data`, | ||
| // so subtraction results in the beginning of the `ArcInner`. |
There was a problem hiding this comment.
I think I still want an explicit line here saying from_raw_inner accepts a pointer to the beginning of the allocation, not the data part.
This is something that is more confusing about this code and while we haven't been great about it I really want this crate to be better on this axis.
Sorry about the repeated back and forth on this!
There was a problem hiding this comment.
Published another version!
`ptr.byte_sub` is stable since 1.75.0. The tricky part here is figuring out offset of `ArcInner.data` for DST. This PR assumes that raw pointer is coming from existing `Arc` so it is safe to convert to reference. I'm not 100% sure this is always correct. Correct version would require `Layout::for_value_raw` (or `mem::align_of_val_raw`) which is not stable. This can be used to convert `Arc<String>` to `Arc<dyn Debug>`.
stepancheg
force-pushed
the
arc-from-raw-unsized
branch
from
4a8cbe7
to
2de2ca4
Compare
|
Thanks! |
ptr.byte_subis stable since 1.75.0.The tricky part here is figuring out offset of
ArcInner.datafor DST. This PR assumes that raw pointer is coming from existingArcso it is safe to convert to reference. I'm not 100% sure this is always correct.Correct version would require
Layout::for_value_raw(ormem::align_of_val_raw) which is not stable.This can be used to convert
Arc<String>toArc<dyn Debug>.