-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Allow Weak::as_ptr and friends for unsized T #74160
Conversation
I assume the plan is to do the same for |
Yes. ... Though to be completely honest, I had convinced myself (somehow) that the sync versions already supported unsized values and didn't need to be tweaked. Obviously that's not true, so I don't know why I thought that... |
The code changes look good to me, but I'd like to see unit tests added that use all of the newly |
Translated the existing into/from raw tests to also do the same for |
5dead90
to
f244725
Compare
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.
👍 from a code perspective.
@rust-lang/libs I think this needs FCP for being an insta-stable change.
@rfcbot fcp merge
|
This comment has been minimized.
This comment has been minimized.
I think I am going to restart this with T-lang on here as well. :/ @rfcbot fcp cancel |
@dtolnay proposal cancelled. |
@rust-lang/libs: This makes the following usable with
@rust-lang/lang: Calling attention to the following callout in the PR description:
(From skimming the implementation I think they might have meant Here is the code in question: Effectively this stabilization is committing to the following semantic being implementable internally to std: unsafe fn align_of_val_raw<T: ?Sized>(val: *const T) -> usize {
if T is Sized {
mem::align_of::<T>()
} else {
mem::align_of_val::<T>(&*val)
}
} Right now we expose the same thing unstably in https://doc.rust-lang.org/nightly/std/mem/fn.align_of_val_raw.html which is tracked by #69835. @rfcbot fcp merge |
Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
Ah, yes, only |
Notably, |
Ah, I see, the key point is that the pointee may be dangling if |
Removing nominated label as this was discussed in today's @rust-lang/lang meeting. No concerns were raised. |
☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts. |
@CAD97 I'll just use GH suggestions to change "iff" to "if" and land that. |
@bors r+ |
📌 Commit e27ef13 has been approved by |
Allow Weak::as_ptr and friends for unsized T Relaxes `impl<T> Weak<T>` to `impl<T: ?Sized> Weak<T>` for the methods `rc::Weak::as_ptr`, `into_raw`, and `from_raw`. Follow-up to rust-lang#73845, which did most of the impl work to make these functions work for `T: ?Sized`. We still have to adjust the implementation of `Weak::from_raw` here, however, because I missed a use of `ptr.is_null()` previously. This check was necessary when `into`/`from_raw` were first implemented, as `into_raw` returned `ptr::null()` for dangling weak. However, we now just (wrapping) offset dangling weaks' pointers the same as nondangling weak, so the null check is no longer necessary (or even hit). (I can submit just 17a928f as a separate PR if desired.) As a nice side effect, moves the `fn is_dangling` definition closer to `Weak::new`, which creates the dangling weak. This technically stabilizes that "something like `align_of_val_raw`" is possible to do. However, I believe the part of the functionality required by these methods here -- specifically, getting the alignment of a pointee from a pointer where it may be dangling iff the pointee is `Sized` -- is uncontroversial enough to stabilize these methods without a way to implement them on stable Rust. r? @RalfJung, who reviewed rust-lang#73845. ATTN: This changes (relaxes) the (input) generic bounds on stable fn!
☀️ Test successful - checks-actions, checks-azure |
Sorry I wasn't able to make the change myself; I've "just" (August... wow time is fast) started grad school and that's eating up all my time now 😅 |
No worries, and enjoy grad school. :) |
…RalfJung Re-stabilize Weak::as_ptr and friends for unsized T As per [T-lang consensus](https://hackmd.io/7r3_is6uTz-163fsOV8Vfg), this uses a branch to handle the dangling case. The discussed optimization of only doing the branch in the T: ?Sized case is left for a followup patch, as doing so is not trivial (as it requires specialization) and not _obviously_ better (as it requires using `wrapping_offset` rather than `offset` more). <details><summary>Basically said optimization</summary> Specialize on `T: Sized`: ```rust fn as_ptr(&self) -> *const T { if [ T is Sized ] || !is_dangling(ptr) { (ptr as *mut T).set_ptr_value( (ptr as *mut u8).wrapping_offset(data_offset) ) } else { ptr::null() } } fn from_raw(*const T) -> Self { if [ T is Sized ] || !ptr.is_null() { let ptr = (ptr as *mut RcBox).set_ptr_value( (ptr as *mut u8).wrapping_offset(-data_offset) ); Weak { ptr } } else { Weak::new() } } ``` (but with more `set_ptr_value` to avoid `Sized` restrictions and maintain metadata.) Written in this fashion, this is not a correctness-critical specialization (i.e. so long as `[ T is Sized ]` is false for unsized `T`, it can be `rand()` for sized `T` without breaking correctness), but it's still touchy, so I'd rather do it in another PR with separate review. --- </details> This effectively reverts rust-lang#80422 and re-establishes rust-lang#74160. T-libs [previously signed off](rust-lang#74160 (comment)) on this stable API change in rust-lang#74160.
…RalfJung Re-stabilize Weak::as_ptr and friends for unsized T As per [T-lang consensus](https://hackmd.io/7r3_is6uTz-163fsOV8Vfg), this uses a branch to handle the dangling case. The discussed optimization of only doing the branch in the T: ?Sized case is left for a followup patch, as doing so is not trivial (as it requires specialization) and not _obviously_ better (as it requires using `wrapping_offset` rather than `offset` more). <details><summary>Basically said optimization</summary> Specialize on `T: Sized`: ```rust fn as_ptr(&self) -> *const T { if [ T is Sized ] || !is_dangling(ptr) { (ptr as *mut T).set_ptr_value( (ptr as *mut u8).wrapping_offset(data_offset) ) } else { ptr::null() } } fn from_raw(*const T) -> Self { if [ T is Sized ] || !ptr.is_null() { let ptr = (ptr as *mut RcBox).set_ptr_value( (ptr as *mut u8).wrapping_offset(-data_offset) ); Weak { ptr } } else { Weak::new() } } ``` (but with more `set_ptr_value` to avoid `Sized` restrictions and maintain metadata.) Written in this fashion, this is not a correctness-critical specialization (i.e. so long as `[ T is Sized ]` is false for unsized `T`, it can be `rand()` for sized `T` without breaking correctness), but it's still touchy, so I'd rather do it in another PR with separate review. --- </details> This effectively reverts rust-lang#80422 and re-establishes rust-lang#74160. T-libs [previously signed off](rust-lang#74160 (comment)) on this stable API change in rust-lang#74160.
Relaxes
impl<T> Weak<T>
toimpl<T: ?Sized> Weak<T>
for the methodsrc::Weak::as_ptr
,into_raw
, andfrom_raw
.Follow-up to #73845, which did most of the impl work to make these functions work for
T: ?Sized
.We still have to adjust the implementation of
Weak::from_raw
here, however, because I missed a use ofptr.is_null()
previously. This check was necessary wheninto
/from_raw
were first implemented, asinto_raw
returnedptr::null()
for dangling weak. However, we now just (wrapping) offset dangling weaks' pointers the same as nondangling weak, so the null check is no longer necessary (or even hit). (I can submit just 17a928f as a separate PR if desired.)As a nice side effect, moves the
fn is_dangling
definition closer toWeak::new
, which creates the dangling weak.This technically stabilizes that "something like
align_of_val_raw
" is possible to do. However, I believe the part of the functionality required by these methods here -- specifically, getting the alignment of a pointee from a pointer where it may be dangling iff the pointee isSized
-- is uncontroversial enough to stabilize these methods without a way to implement them on stable Rust.r? @RalfJung, who reviewed #73845.
ATTN: This changes (relaxes) the (input) generic bounds on stable fn!