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 project_replace method. #194

Merged
merged 3 commits into from
May 3, 2020
Merged

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Apr 26, 2020

I implemented this as an opt-in method, via #[pin_project(Replace)] because it requires Self: Sized, and because I thought it would be safest to disallow using in combination with PinnedDrop (as it would effectively bypass the PinnedDrop implementation).

Closes #184

@Diggsey Diggsey requested a review from taiki-e as a code owner April 26, 2020 21:44
@taiki-e
Copy link
Owner

taiki-e commented Apr 29, 2020

Thanks for the PR! Could you add documentation?

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 29, 2020

Done 👍

Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

LGTM aside from a few nits.

pin-project-internal/src/pin_project/derive.rs Outdated Show resolved Hide resolved
pin-project-internal/src/pin_project/derive.rs Outdated Show resolved Hide resolved
let proj_ty_generics = self.proj.generics.split_for_impl().1;
let (impl_generics, ty_generics, where_clause) = self.orig.generics.split_for_impl();

let replace_impl = self.replace.map(|replace| {
quote_spanned! { replace =>
#[allow(unsafe_code)]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
#[allow(unsafe_code)]

#[deny(unsafe_code)] does not deny unsafe code generated by macros (rust-lang/rust#53975)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I only added this because it failed the test suite without it, so it doesn't appear like that behaviour is working correctly.

@Diggsey
Copy link
Contributor Author

Diggsey commented May 3, 2020

@taiki-e I've added the workaround for the compiler bug. I didn't remove the #[allow(unsafe_code)] attribute because it does seem to be required (I think because I'm using quote_spanned! instead of quote!.)

@taiki-e
Copy link
Owner

taiki-e commented May 3, 2020

Ok! I will look into the span issues after merging this PR.

@taiki-e
Copy link
Owner

taiki-e commented May 3, 2020

bors r+

@taiki-e
Copy link
Owner

taiki-e commented May 3, 2020

Hmm... bors seems to be crashed, I'll merge via the merge button.

@taiki-e taiki-e merged commit 61d9d17 into taiki-e:master May 3, 2020
@taiki-e
Copy link
Owner

taiki-e commented May 4, 2020

Published in 0.4.10.

@Diggsey Diggsey deleted the project-replace branch May 4, 2020 13:03
@taiki-e taiki-e added the A-pin-projection Area: #[pin_project] label May 7, 2020
bors bot added a commit that referenced this pull request May 8, 2020
212: Add test for project_replace on panic r=taiki-e a=taiki-e

Refs: 
* rust-lang/rust#47949
* #194 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@taiki-e taiki-e added A-project_replace Area: project_replace method and removed A-pin-projection Area: #[pin_project] labels Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-project_replace Area: project_replace method
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement replace() method to move out of struct/enum
2 participants