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

Tracking issue for drop_in_place #27908

Closed
Gankra opened this issue Aug 19, 2015 · 11 comments
Closed

Tracking issue for drop_in_place #27908

Gankra opened this issue Aug 19, 2015 · 11 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Gankra
Copy link
Contributor

Gankra commented Aug 19, 2015

This calls the "drop glue" for a given type without having to explicitly read it out onto the stack to be dropped. All types have a drop glue implementation, which involves calling the types Drop impl (if it exists), and the recursively running drop glue on its fields.

drop_in_place is both a performance improvement (in the case of e.g. Vec, it avoids ptr::reading the elements out), and a semantic necessity (in the case of DSTs, it's impossible to ptr::read the value, but the vtable always contains a drop glue impl).

It is currently exposed in the ptr module because it's maximally expressive to take a *mut and because "that's what the intrinsic took" at the time it was exposed outside of std::intrinsics. It may be desirable to instead expose it in mem to parallel mem::drop, and require an &mut. For most cases you probably already have an &mut anyway, and it's trivial to upgrade.

@Gankra Gankra added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Aug 19, 2015
@Marwes
Copy link
Contributor

Marwes commented Jan 10, 2016

Are there any plans to stabilize this?

@Gankra
Copy link
Contributor Author

Gankra commented Jan 10, 2016

@nagy24 it seems your message got garbbled up?

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 10, 2016
@arielb1
Copy link
Contributor

arielb1 commented Jan 11, 2016

This won't really work with &mut because it will leave it pointing to an invalid value.

@Gankra
Copy link
Contributor Author

Gankra commented Jan 12, 2016

@arielb1 it would definitely have to be unsafe. Making the value invalid isn't necessarily a problem unless you're picturing more advanced analysis. I do kind've like the idea of a convention where invalidating ops are only exposed on raw pointers, though.

@alexcrichton
Copy link
Member

🔔 This issue is now entering its cycle final comment period to be stabilization in 1.8 🔔

The libs team is somewhat up in the air about whether this function should take *mut T vs &mut T. If it were to take &mut T it would be in the mem module instead of the ptr module.

One one hand this method is only actually safe to call if you have a valid unique reference (e.g. &mut T). On the other hand, however, the reference is immediately invalid to use right after the call.

We're curious if others have opinions!

@alexcrichton alexcrichton added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed I-nominated labels Jan 29, 2016
@KalitaAlexey
Copy link
Contributor

Is there a lint to warn about use after drop?

@Marwes
Copy link
Contributor

Marwes commented Jan 29, 2016

Since drop and forget is in the mem module I would argue for drop_in_place to be there as well. Even though it is an unsafe intrinsic instead of a safe convenience function it still interacts with the Drop trait just as drop and forget does. On the other hand the ptr module only indirectly interacts with Drop.

For the choice between &mut and *mut I have less of an opinion. I were going to say that there are already Drop implementations that invalidate the &mut self pointer but I couldn't actually think of a case where this happens. Virtually all Drop implementations have a *mut which their memory is allocated behind which becomes invalid but the &mut self itself pointer is always valid for types such as Vec, HashMap etc.

@alexcrichton
Copy link
Member

@KalitaAlexey not currently, no.

@Marwes perhaps yeah, although the placement of mem vs ptr today currently just largely has to do with the types of the arguments (e.g. raw pointers or "safe" values). Basically if we stabilize with *mut T it would be unconventional to place it in the mem module (and vice versa if we stabilize with &mut T).

My thinking for using a raw pointer is that if you're calling drop_in_place you're pretty unlikely to have a safe mutable pointer to the data in the first place anyway. For example the Rc and Arc both have to go through a *mut RcBox<T> to get at the underlying value.

@briansmith
Copy link
Contributor

This calls the "drop glue" for a given type without having to explicitly read it out onto the stack to be dropped.

Why not change how drop is optimized to avoid the copy? It seems such an optimization is needed by many things.

@alexcrichton
Copy link
Member

@briansmith this isn't necessarily related to compiler optimizations, it's just replacing the old method of dropping a value by doing so via ptr::read. The compiler already doesn't copy anything around to call destructors, it just prevents calling the drop method directly. This is essentially providing the ability to do so, but it works in a generic context and works with trait objects. as well.

@alexcrichton
Copy link
Member

The libs team discussed this during triage yesterday and the conclusion was to stabilize in the ptr module.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Feb 29, 2016
This commit is the result of the FCPs ending for the 1.8 release cycle for both
the libs and the lang suteams. The full list of changes are:

Stabilized

* `braced_empty_structs`
* `augmented_assignments`
* `str::encode_utf16` - renamed from `utf16_units`
* `str::EncodeUtf16` - renamed from `Utf16Units`
* `Ref::map`
* `RefMut::map`
* `ptr::drop_in_place`
* `time::Instant`
* `time::SystemTime`
* `{Instant,SystemTime}::now`
* `{Instant,SystemTime}::duration_since` - renamed from `duration_from_earlier`
* `{Instant,SystemTime}::elapsed`
* Various `Add`/`Sub` impls for `Time` and `SystemTime`
* `SystemTimeError`
* `SystemTimeError::duration`
* Various impls for `SystemTimeError`
* `UNIX_EPOCH`
* `ops::{Add,Sub,Mul,Div,Rem,BitAnd,BitOr,BitXor,Shl,Shr}Assign`

Deprecated

* Scoped TLS (the `scoped_thread_local!` macro)
* `Ref::filter_map`
* `RefMut::filter_map`
* `RwLockReadGuard::map`
* `RwLockWriteGuard::map`
* `Condvar::wait_timeout_with`

Closes rust-lang#27714
Closes rust-lang#27715
Closes rust-lang#27746
Closes rust-lang#27748
Closes rust-lang#27908
Closes rust-lang#29866
bors added a commit that referenced this issue Feb 29, 2016
This commit is the result of the FCPs ending for the 1.8 release cycle for both
the libs and the lang suteams. The full list of changes are:

Stabilized

* `braced_empty_structs`
* `augmented_assignments`
* `str::encode_utf16` - renamed from `utf16_units`
* `str::EncodeUtf16` - renamed from `Utf16Units`
* `Ref::map`
* `RefMut::map`
* `ptr::drop_in_place`
* `time::Instant`
* `time::SystemTime`
* `{Instant,SystemTime}::now`
* `{Instant,SystemTime}::duration_since` - renamed from `duration_from_earlier`
* `{Instant,SystemTime}::elapsed`
* Various `Add`/`Sub` impls for `Time` and `SystemTime`
* `SystemTimeError`
* `SystemTimeError::duration`
* Various impls for `SystemTimeError`
* `UNIX_EPOCH`
* `ops::{Add,Sub,Mul,Div,Rem,BitAnd,BitOr,BitXor,Shl,Shr}Assign`

Deprecated

* Scoped TLS (the `scoped_thread_local!` macro)
* `Ref::filter_map`
* `RefMut::filter_map`
* `RwLockReadGuard::map`
* `RwLockWriteGuard::map`
* `Condvar::wait_timeout_with`

Closes #27714
Closes #27715
Closes #27746
Closes #27748
Closes #27908
Closes #29866
Closes #28235
Closes #29720
dlrobertson pushed a commit to dlrobertson/rust that referenced this issue Nov 29, 2018
This commit is the result of the FCPs ending for the 1.8 release cycle for both
the libs and the lang suteams. The full list of changes are:

Stabilized

* `braced_empty_structs`
* `augmented_assignments`
* `str::encode_utf16` - renamed from `utf16_units`
* `str::EncodeUtf16` - renamed from `Utf16Units`
* `Ref::map`
* `RefMut::map`
* `ptr::drop_in_place`
* `time::Instant`
* `time::SystemTime`
* `{Instant,SystemTime}::now`
* `{Instant,SystemTime}::duration_since` - renamed from `duration_from_earlier`
* `{Instant,SystemTime}::elapsed`
* Various `Add`/`Sub` impls for `Time` and `SystemTime`
* `SystemTimeError`
* `SystemTimeError::duration`
* Various impls for `SystemTimeError`
* `UNIX_EPOCH`
* `ops::{Add,Sub,Mul,Div,Rem,BitAnd,BitOr,BitXor,Shl,Shr}Assign`

Deprecated

* Scoped TLS (the `scoped_thread_local!` macro)
* `Ref::filter_map`
* `RefMut::filter_map`
* `RwLockReadGuard::map`
* `RwLockWriteGuard::map`
* `Condvar::wait_timeout_with`

Closes rust-lang#27714
Closes rust-lang#27715
Closes rust-lang#27746
Closes rust-lang#27748
Closes rust-lang#27908
Closes rust-lang#29866
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants