PyObjectRc: thin wrapper for PyObjectRef inner Rc #2155
Conversation
e2df7d4 to
454d132
Compare
vm/src/pyobject.rs
Outdated
| @@ -472,9 +475,9 @@ impl PyObject<dyn PyObjectPayload> { | |||
| self: PyRc<Self>, | |||
There was a problem hiding this comment.
Can this be changed to take self: PyObjectRc<Self> instead?
There was a problem hiding this comment.
Nope -- blocked on the arbitrary_self_types feature
4bf87fd to
4a29357
Compare
|
this generic way seems not fit for drop implementation 🤦 |
|
What if you make it generic over the payload for PyObject? e.g. |
|
I think that would make the name make more sense as well; PyObjectRc is a wrapper around |
fd705bd to
099ce1d
Compare
|
@coolreader18 @Skinny121 thanks for the reviews! I think this is finally done |
cc1c9a7 to
273fcb2
Compare
|
after converting to embed PyObject, it was possible to move some parts back to pyobject.rs |
vm/src/pyobjectrc.rs
Outdated
| T: ?Sized + PyObjectPayload, | ||
| PyRc<PyObject<T>>: AsPyObjectRef, | ||
| { | ||
| type Target = PyRc<PyObject<T>>; |
There was a problem hiding this comment.
If we're making it unsafe to access the inner PyRc, we should probably make the target PyObject<T>
There was a problem hiding this comment.
I think trying to make target PyObject<T> is a good idea. But accessing to inner PyRc is safe. into_rc is unsafe because it skips dropping of PyObjectRc. This is borrowing, so there is no reason to be unsafe.
There was a problem hiding this comment.
oh, it can be a problem when it is cloned from the ref, got it.
273fcb2 to
a13cf32
Compare
vm/src/pyobjectrc.rs
Outdated
| inner: PyRc<PyObject<T>>, | ||
| } | ||
|
|
||
| pub type PyObjectWeak<T = dyn PyObjectPayload> = PyWeak<PyObject<T>>; |
There was a problem hiding this comment.
Should probably be a newtype as well 😬 still the issue of downgrade->upgrade to Arc->drop original PyObjectRc->drop Arc without __del__. It would also let you add the upgrade() method to PyObjectWeak and have it return a PyObjectRc
869f5b8 to
0d6c77c
Compare
Split
__del__unrelated part from #2128this patch will be base of any kind of drop implementation