unsized ManuallyDrop #53033
unsized ManuallyDrop #53033
Conversation
|
r? @bluss (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
cramertj
added
the
T-libs-api
kennytm
added
the
relnotes
src/libcore/mem.rs
Outdated
| #[inline] | ||
| fn deref_mut(&mut self) -> &mut Self::Target { | ||
| &mut self.value | ||
| } | ||
| } | ||
|
|
||
| #[unstable(feature = "coerce_unsized", issue = "27732")] | ||
| impl<T: CoerceUnsized<U>, U> CoerceUnsized<ManuallyDrop<U>> for ManuallyDrop<T> {} |
There was a problem hiding this comment.
Uh, no, this allows ManuallyDrop<Rc<T>> to coerce to ManuallyDrop<Rc<dyn Trait>> - do we need that?
There was a problem hiding this comment.
Is this change even useful without a CoerceUnsized? AFAIK all types that do T: ?Sized also have a CoerceUnsized, or is that not the case?
There was a problem hiding this comment.
In your test, the only needed CoercedUnsized impl is that for Box, pointers to ManuallyDrop can be coerced just because it's a ?Sized struct (you can test this with any such structs, there's no opt-in there).
src/libcore/tests/manually_drop.rs
Outdated
| @@ -21,4 +21,8 @@ fn smoke() { | |||
|
|
|||
| let x = ManuallyDrop::new(TypeWithDrop); | |||
| drop(x); | |||
|
|
|||
| // also test unsizing | |||
| let x : Box<ManuallyDrop<[TypeWithDrop]>> = Box::new(ManuallyDrop::new([TypeWithDrop])); | |||
There was a problem hiding this comment.
This would be less confusing if the array had two elements or something.
|
I also added |
@rfcbot fcp merge |
|
Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once a majority of reviewers approve (and none object), 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. |
rfcbot
added
proposed-final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
added
final-comment-period
|
@bors: r=SimonSapin |
|
📌 Commit 5ee5a7e has been approved by |
bors
added
S-waiting-on-bors
unsized ManuallyDrop I think this matches what @eddyb had in rust-lang#52711 originally. ~~However, I have never added a `CoerceUnsized` before so I am not sure if I did this right. I copied the `unstable` attribute on the `impl` from elsewhere, but AFAIK it is useless because `impl`'s are insta-stable... so shouldn't this rather say "stable since 1.30"?~~ This is insta-stable and hence requires FCP, at least. Fixes rust-lang#47034
unsized ManuallyDrop I think this matches what @eddyb had in #52711 originally. ~~However, I have never added a `CoerceUnsized` before so I am not sure if I did this right. I copied the `unstable` attribute on the `impl` from elsewhere, but AFAIK it is useless because `impl`'s are insta-stable... so shouldn't this rather say "stable since 1.30"?~~ This is insta-stable and hence requires FCP, at least. Fixes #47034
|
☀️ Test successful - status-appveyor, status-travis |
rfcbot
added
finished-final-comment-period
|
Why was no |
|
Those are for smart ptrs only, not for structs that wrap a |
|
To expand on that, we could add this: impl<T, U> CoerceUnsized<ManuallyDrop<U>> for ManuallyDrop<T>
where T: CoerceUnsized<U>
{}But what it'd do is let e.g. |
I think having some sort of coercion like that is not harmful at all. |
I think this matches what @eddyb had in #52711 originally.
However, I have never added aCoerceUnsizedbefore so I am not sure if I did this right. I copied theunstableattribute on theimplfrom elsewhere, but AFAIK it is useless becauseimpl's are insta-stable... so shouldn't this rather say "stable since 1.30"?This is insta-stable and hence requires FCP, at least.
Fixes #47034