Skip to content

Explain drop a bit more #30696

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 19, 2016
Merged

Explain drop a bit more #30696

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/libcore/ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ use fmt;
#[stable(feature = "rust1", since = "1.0.0")]
pub trait Drop {
/// A method called when the value goes out of scope.
///
/// When this method has been called, `self` has not yet been deallocated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tense in the first clause makes things a bit ambiguous. How about something like "Before this method is called, self has not yet been deallocated."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm not sure it even makes sense to use the word "deallocated," since a number of Drop impls don't have anything to do with allocation. I think this description should probably just talk about it being invalid (or some other word) to access self after drop has been called.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I mean, I felt like it was a little weird because &mut is always valid, but @durka felt differently.

/// If it were, `self` would be a dangling reference.
///
/// After this function is over, the memory of `self` will be deallocated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of 'deallocated', 'deinitialized' would be more accurate? It could even say, 'deinitialized, and if self is heap-allocated, deallocated', or 'and possibly deallocated'.

///
/// # Panics
///
/// Given that a `panic!` will call `drop()` as it unwinds, any `panic!` in
/// a `drop()` implementation will likely abort.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of want this to say more. It's still not obvious why panicking during drop results in abort. Is there additional material we can link to that explains exception safety and double-panic?

This could be more emphatic, like

Implementations of drop must not panic. Because drop may be called automatically called while the callstack is unwinding after a panic, panicking during drop is likely to result in a "double panic" (panic during unwinding). For reasons (link), a double panic will cause the process to abort. Therefore, Drop implementations must take care to not panic, alternatives to panicking in drop include ... (link).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the double panic detector; the comment implies that something besides an abort could be done but it would be tricky. Scary-sounding alternatives like calling std::thread::panicking or using panic::recover should maybe be in the Nomicon instead of spelled out here, I would say?

#[stable(feature = "rust1", since = "1.0.0")]
fn drop(&mut self);
}
Expand Down