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

Implement replace() method to move out of struct/enum #184

Closed
Diggsey opened this issue Apr 7, 2020 · 7 comments · Fixed by #194
Closed

Implement replace() method to move out of struct/enum #184

Diggsey opened this issue Apr 7, 2020 · 7 comments · Fixed by #194
Labels
A-pin-projection Area: #[pin_project] C-enhancement Category: A new feature or an improvement for an existing one

Comments

@Diggsey
Copy link
Contributor

Diggsey commented Apr 7, 2020

Replacing a pinned value is safe as long as the destructors of types which do not implement Unpin are called.

Pin::set allows replacing the whole value, but it does not allow recovering the un-pinned fields, so the idea is to provide a method to achieve this.

The API would look something like:

#[pin_project]
enum Example {
    A(#[pin] PinnedValue),
    B(UnpinnedValue),
    C,
}

...

#[project]
match self.replace_and_project(Example::C) {
    Example::A() => { /* no data */ },
    Example::B(value) => { /* oh look a value! */ },
    Example::C => {},
}
@Aaron1011
Copy link
Collaborator

I was working on something very similar to this, but never quite finished. I can finish it up and open a PR.

@taiki-e
Copy link
Owner

taiki-e commented Apr 10, 2020

It seems that this can be replaced by wrapping the unpinned field with an Option.

#[pin_project]
enum Example {
    A(#[pin] PinnedValue),
    B(Option<UnpinnedValue>),
    C,
}

#[project]
match self.as_mut().project() {
    Example::A(_) => { /* ... */ },
    Example::B(value @ Some(_)) => { 
        let value = value.take().unwrap();
        // ... 
    },
    Example::B(None) | Example::C => {},
}
self.set(Example::C); // When using this way, whether you should replace self depends on the situation.

@taiki-e taiki-e added the C-enhancement Category: A new feature or an improvement for an existing one label Apr 10, 2020
@taiki-e
Copy link
Owner

taiki-e commented Apr 25, 2020

It seems reasonable to add this, given that it increases the size if use Option and that we cannot easily change the public type signature.

@taiki-e
Copy link
Owner

taiki-e commented Apr 25, 2020

Assigning to @Aaron1011 based on the previous comment (feel free to unassign).

@taiki-e
Copy link
Owner

taiki-e commented Apr 25, 2020

IIUC, macro needs to generate an additional struct/enum to support this.

@Aaron1011: Do you think it preferable to add a feature like #124 to implement this? (I feel the existing project-attribute-based approach becomes bothered as more types are generated.)

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 25, 2020

In order to be panic-safe, it will need to generate code something like:

struct DropInPlaceHelper<T>(*mut T);

impl<T> DropInPlaceHelper<T> {
    fn drop(&mut self) {
        ptr::drop_in_place(self.0);
    }
}

struct OverwriteHelper<T>(*mut T, MaybeUninit<T>);

impl<T> OverwriteHelper<T> {
    fn drop(&mut self) {
        ptr::write(self.0, self.1.assume_init());
    }
}

fn partial_replace(ptr: *mut Foo, new_value: Foo) -> Y {
    let _helper0 = OverwriteHelper(ptr, MaybeUninit::new(new_value));
    let _helper1 = DropInPlaceHelper(&mut (*ptr).pinned_field1);
    let _helper2 = DropInPlaceHelper(&mut (*ptr).pinned_field2);
    let _helper3 = DropInPlaceHelper(&mut (*ptr).pinned_field3);
    ptr::read(&mut (*ptr).unpinned_field)
}

@taiki-e
Copy link
Owner

taiki-e commented Apr 25, 2020

(Maybe we need to wait for rust-lang/unsafe-code-guidelines#232 discussion?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pin-projection Area: #[pin_project] C-enhancement Category: A new feature or an improvement for an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants