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

Add drop_leak and drop_leak_mut #83786

Closed
wants to merge 2 commits into from
Closed

Conversation

davidspies
Copy link

@davidspies davidspies commented Apr 2, 2021

I'm trying to implement a StaticRef type which can be built from Rc<RefCell<T>> without needing a lifetime (when dropped, it decrements the refcount and the borrow count). But I find that undo_leak doesn't quite do what I want it to do in the presence of multiple StaticRefs, so I'd like to add drop_leak.

Here's the implementation of StaticRef:

#![feature(cell_leak)]

use std::{
    cell::{Ref, RefCell},
    ops::Deref,
    rc::Rc,
};

pub struct StaticRef<T, S = T>(Holder<S>, *const T);

impl<T> StaticRef<T> {
    pub fn borrow_from(orig: Rc<RefCell<T>>) -> Self {
        let x: *const T = Ref::leak(orig.borrow());
        StaticRef(Holder(orig), x)
    }
}
impl<S, T> StaticRef<T, S> {
    pub fn map<T2, F: for<'b> FnOnce(&'b T) -> &'b T2>(self, op: F) -> StaticRef<T2, S> {
        let StaticRef(holder, x) = self;
        StaticRef(holder, op(unsafe { &*x }))
    }
}

struct Holder<S>(Rc<RefCell<S>>);

impl<S> Drop for Holder<S> {
    fn drop(&mut self) {
        unsafe { self.0.drop_leak() }
    }
}

impl<S, T> Deref for StaticRef<T, S> {
    type Target = T;

    fn deref<'b>(&'b self) -> &'b T {
        unsafe { &*self.1 }
    }
}

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 2, 2021
@rust-log-analyzer

This comment has been minimized.

@davidspies
Copy link
Author

How do I run just the two doctests locally? It seems to take forever to build and test any changes

@SkiFire13
Copy link
Contributor

I guess #69099 should be updated to include this PR

@kennytm
Copy link
Member

kennytm commented Apr 4, 2021

How do I run just the two doctests locally? It seems to take forever to build and test any changes

i think it's easier to copy the entire RefCell into its own crate outside of libstd during development.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 30, 2021
/// assert!(c.try_borrow_mut().is_err());
/// unsafe { c.drop_leak() };
/// assert!(c.try_borrow_mut().is_ok());
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

We should add a panic section to describe when this panics.

/// assert!(c.try_borrow().is_err());
/// unsafe { c.drop_leak_mut() };
/// assert!(c.try_borrow().is_ok());
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2021
@JohnCSimon
Copy link
Member

Ping from triage
@davidspies can you please address the reviewer comments? Thank you.

@crlf0710
Copy link
Member

@davidspies Ping from triage, any updates on this?

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2021
@crlf0710 crlf0710 added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 14, 2021
@crlf0710
Copy link
Member

Triage: I'm closing this due to inactivity. Feel free to reopen or create a new PR when you're ready to work on this again, thanks!

@crlf0710 crlf0710 closed this Aug 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants