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

Fix fallout from #57667 #58021

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@ishitatsuyuki
Copy link
Member

ishitatsuyuki commented Jan 31, 2019

r? @eddyb

Verified in miri.

drop(Box::from_raw(p));
// Free the allocated memory only,
// do not drop T as it was ptr::read above.
drop(Box::from_raw(p as *mut ManuallyDrop<T>));

This comment has been minimized.

@eddyb

eddyb Jan 31, 2019

Member

cc @RalfJung this is a clever trick I didn't consider!

This comment has been minimized.

@RalfJung

RalfJung Jan 31, 2019

Member

Yeah I think this should work. I don't like the raw ptr cast though, it's so easy to get those wrong -- but I also don't know another way here.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Jan 31, 2019

r=me if @RalfJung can confirm it's a correct approach

@ishitatsuyuki

This comment has been minimized.

Copy link
Member Author

ishitatsuyuki commented Feb 3, 2019

Do you suggestions on alternatives to raw pointer cast? If not, I guess the code should be merged as-is.

@ollie27

This comment has been minimized.

Copy link
Contributor

ollie27 commented Feb 8, 2019

This might be a shot in the dark but would something like the following work (and deal with the FIXME):

pub fn filter_map<F>(self, f: F) -> Option<P<T>> where
    F: FnOnce(T) -> Option<T>,
{
    unsafe {
        // Transmute to `Box<MaybeUninit<T>>` so if `f` `panic`s or returns
        // `None` the `Box` will be freed but not the inner T.
        let mut maybe_box: Box<MaybeUninit<T>> = mem::transmute(self.ptr);
        let x = f(ptr::read(maybe_box.as_ptr()))?;
        maybe_box.set(x);
        Some(P { ptr: mem::transmute(maybe_box) })
    }
}
@ishitatsuyuki

This comment has been minimized.

Copy link
Member Author

ishitatsuyuki commented Feb 10, 2019

I think @ollie27's suggestion is pretty good, any opinions/alternatives regarding the use of transmute?

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Feb 10, 2019

It seems to me that using ManuallyDrop instead of MaybeUninit is sufficient?

@ishitatsuyuki

This comment has been minimized.

Copy link
Member Author

ishitatsuyuki commented Feb 10, 2019

@RalfJung the snippet above uses MaybeUninit::set.

@ollie27

This comment has been minimized.

Copy link
Contributor

ollie27 commented Feb 10, 2019

I guess this would work:

pub fn filter_map<F>(self, f: F) -> Option<P<T>> where
    F: FnOnce(T) -> Option<T>,
{
    unsafe {
        // Transmute to `Box<ManuallyDrop<T>>` so if `f` `panic`s or returns
        // `None` the `Box` will be freed but not the inner T.
        let mut manaully_drop_box: Box<ManuallyDrop<T>> = mem::transmute(self.ptr);
        let x = f(ManuallyDrop::take(&mut manaully_drop_box))?;
        *manaully_drop_box = ManuallyDrop::new(x);
        Some(P { ptr: mem::transmute(manaully_drop_box) })
    }
}
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Feb 10, 2019

Yeah, something like that.

Either way though I think it'd be good to avoid "open" transmute and use helper functions instead. Turning Box<MaybeUninit<T>> into Box<T> should probably even become a stable (unsafe) operation some day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment