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

[WIP] FromPyObject for #[pyclass] with T: Clone #730

Merged
merged 1 commit into from
Jan 25, 2020

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jan 13, 2020

I had an idea how to support extracting for pyclasses automatically for T: Clone.

It also has really nice error messages if the user attempts to extract a non-clone #[pyclass] (see the ui test).

Closes #706

@kngwyu
Copy link
Member

kngwyu commented Jan 14, 2020

Nice 👍 , thank you.
But please wait for the review for a while, since I want to check we can simplify the implementation or not.

@davidhewitt
Copy link
Member Author

davidhewitt commented Jan 14, 2020

Sure thing, take as long as you like!

I tried myself to do some simplifications but couldn't get much further than what's above.

It is possible to add some blanket impl FromPyObjectImpl for &T and &mut T, for example. But it's not possible to also have a blanket impl for T because it conflicts with the other two.

@davidhewitt
Copy link
Member Author

I did spot a slight simplification!

Also, I realised that if I work the FromPyObject implementation for Vec<T> using this new FromPyObjectImpl trait, we can get the buffer::Element optimization without specialization. See davidhewitt#1

It isn't perfect and it's really un-ergonomic to have to implement FromPyObjectImpl for everything to get Vec<T> to work.

So we might want to think a bit longer about this idea. I think if we can find a nice API for it then we can use it to remove specialization from impl FromPyObject for Vec<T>.

@davidhewitt davidhewitt changed the title FromPyObject for #[pyclass] with T: Clone [WIP] FromPyObject for #[pyclass] with T: Clone Jan 14, 2020
@kngwyu
Copy link
Member

kngwyu commented Jan 18, 2020

Sorry for the lazy review, but after playing around the code, I'm really convinced that this is almost the minimal change 👍
All my review suggestions are optional and I don't have strong opinions about them.

src/conversion.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/conversion.rs Show resolved Hide resolved
@kngwyu kngwyu added this to the 0.9.0 milestone Jan 19, 2020
@kngwyu
Copy link
Member

kngwyu commented Jan 23, 2020

Just a note:
I added this PR to 0.9.0 milestone since somehow rust-numpy needs this fix.

@davidhewitt
Copy link
Member Author

Ok. I'm closing in on a design I like for the Vec<T> specialization solution (see the PR I linked in comment above).

Once that's ready I'll open it as a PR against pyO3 and then we can choose whether to take this simpler change, or the bigger design change from that PR.

Copy link
Member Author

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

I've addressed all your comments and squashed to a single commit. One thing I've pointed out for you but otherwise this is ready to merge.

Having thought about the Vec<T> specialization more, the design I've settled on does not impact this design any more. So please merge this PR without waiting for whatever I come up with there.

@@ -152,7 +152,7 @@ pub mod buffer;
#[doc(hidden)]
pub mod callback;
pub mod class;
mod conversion;
pub mod conversion;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only change which you might want to check. I think this is acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

👍

@kngwyu
Copy link
Member

kngwyu commented Jan 25, 2020

Thank you!

@kngwyu kngwyu merged commit 541816b into PyO3:master Jan 25, 2020
@davidhewitt davidhewitt deleted the extract-clone branch August 10, 2021 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically implement Clone based conversion
2 participants