avoid creating PyRef inside __traverse__ handler#4479
Conversation
| /// Error returned by a `__traverse__` visitor implementation. | ||
| #[repr(transparent)] | ||
| pub struct PyTraverseError(pub(crate) c_int); | ||
| pub struct PyTraverseError(get_nonzero_c_int::TYPE); |
There was a problem hiding this comment.
A bit of a drive-by optimization, to add a niche for the 0.
| /// Prevents the `PyVisit` from outliving the `__traverse__` call. | ||
| pub(crate) _guard: PhantomData<&'a ()>, |
There was a problem hiding this comment.
I completely removed the Python::assume_gil_acquired call from _call_traverse, so I needed to place a different reference back in for the lifetime. This is sufficient because the *mut c_void already prevents Send/Sync.
|
cc @ngoldbaum |
|
We could also make the change suggested by the comment here.... #[repr(transparent)]
pub struct PyRef<'p, T: PyClass> {
// TODO: once the GIL Ref API is removed, consider adding a lifetime parameter to `PyRef` to
// store `Borrowed` here instead, avoiding reference counting overhead.
inner: Bound<'p, T>,
}as that would also avoid changing reference counts. |
|
I believe that would need #4390. Also in that thread #4390 (comment) there was the suggestion to maybe delay that change to 0.24 |
|
Yes agreed, I considered changing I also think I might backport this fix to 0.22, which would be another reason not to have the breaking change here. |
1331d09 to
1da0de8
Compare
|
I did some local testing on this PR. After rebasing on |
|
Great, will merge this. I am considering landing this as part of a stack of fixes in a 0.22.3 patch. |
See #4265 (comment)
We cannot use
PyRef<T>to guard access to the value inside a__traverse__handler. Instead I use thePyClassObject<T>and manually perform the borrow checking.