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

Remove ToPyPointer and so on from pyclass #335

Merged
merged 10 commits into from
Feb 12, 2019
Merged

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Feb 1, 2019

And introduce PyRef and PyRefMut.
See #320 for the reason.

Currently I don't have good idea for PyIterProtocol test and it still fails to be compiled.

Fixes #308

@konstin
Copy link
Member

konstin commented Feb 1, 2019

Thank you, the two types are great!

Both structs should get a docstring with an example, a note in the guide and a conversion from PyObject to that struct, so that it can be used in arguments.

For PyIterProtocol, I think that the current signature is wrong. There might be a better solution with arbitrary_self_types, but for now passing self as not-self parameter to __iter__ might be the best solution.

@kngwyu kngwyu changed the title Remove ToPyPointer and so on from pyclass [WIP] Remove ToPyPointer and so on from pyclass Feb 2, 2019
@kngwyu
Copy link
Member Author

kngwyu commented Feb 2, 2019

There might be a better solution with arbitrary_self_types

You mean self: Py<Self> or so?
It seems really good except it's unstable :sad:

for now passing self as not-self parameter to iter might be the best solution

Thanks, I'll try this.

@kngwyu
Copy link
Member Author

kngwyu commented Feb 5, 2019

Just completed fixing tests, please wait a little for documents.

@kngwyu kngwyu changed the title [WIP] Remove ToPyPointer and so on from pyclass Remove ToPyPointer and so on from pyclass Feb 7, 2019
@kngwyu
Copy link
Member Author

kngwyu commented Feb 7, 2019

Ready to be reviewed now.
To avoid confusion, I modified AsPyRef to return PyRef.
By this change, now PyObject::as_ref returns PyRef<PyObjectRef>.
It's not desirable, but I think it's not so much confusing because I also modified PyTryFrom so that it can take PyRef and PyRefMut.

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

Added some minor stuff, otherwise it's looking good!

Three more things that have no line they belong to:

  • The guide should say something about those types. Mainly explain that those two are meant to ensure that an instance of a rust struct is actually part of an python object and that you need ot use Py<T> to store an object for longer than the GIL lifetime
  • Are there Ts for PyRef that do not implement PyTypeInfo?
  • PyRef should be neither Send not Sync (e.g. adding a PhantomData<Rc<()>> field achieves that; Negative impls seem to be still unstable)

src/instance.rs Outdated Show resolved Hide resolved
src/instance.rs Outdated Show resolved Hide resolved
src/instance.rs Outdated Show resolved Hide resolved
src/instance.rs Outdated Show resolved Hide resolved
src/instance.rs Show resolved Hide resolved
src/instance.rs Outdated Show resolved Hide resolved
src/instance.rs Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member Author

kngwyu commented Feb 8, 2019

Thanks for your review!
I refactored codes based on your comments, though still I haven't completed the guide.

Are there Ts for PyRef that do not implement PyTypeInfo?

I think there aren't so added a bound to PyRef itself.

@kngwyu
Copy link
Member Author

kngwyu commented Feb 8, 2019

Write a draft of document, and removed Py::new_ref in favor of PyRef::new.
I'm sorry this change may conflicts with #344, but I think we need this, because Py::new_ref is not related to Py.

@@ -16,6 +16,68 @@ struct MyClass {

The above example generates implementations for `PyTypeInfo` and `PyTypeObject` for `MyClass`.

## Get Python objects from `pyclass`
In rust side, we can get a Python object which includes `pyclass` by 3 methods.
Copy link
Member

Choose a reason for hiding this comment

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

This needs a short explanation that if you instantiate a rust struct, it doesn't need to be part of a python object a python object. PyRef however guarantees that the struct instance is part of a python object and that you're also holding the GIL, which allows you to modify the struct.

Everything else looks good!

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@konstin
Copy link
Member

konstin commented Feb 10, 2019

I'll rebase #344 onto this once it is merged to master, it shouldn't be much more than a bit of search an replace.

@konstin konstin merged commit 1d17463 into PyO3:master Feb 12, 2019
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.

2 participants