Move PyTypeInfo::AsRefTarget onto new trait HasPyGilBoundRef #2956
Conversation
| /// # Safety | ||
| /// `Py<T>` will hand out references to `Self::AsRefTarget`. This _must_ have the | ||
| /// same layout as `*mut ffi::PyObject`. | ||
| pub unsafe trait HasPyGilBoundRef { |
There was a problem hiding this comment.
IMO the name could use some work. I don't have a name that is obviously good, but what about something like AsGilBorrow?
There was a problem hiding this comment.
Sure thing. I think we can sit on this and think for a bit. I'm a bit lost for a good name too. StorableInPy?
There was a problem hiding this comment.
Just to add completely different colour for the bikeshed: PyHeapType meaning that can be stored on the Python heap (i.e. in Py<T>) and thereby shared references must be bound to the GIL (as it protects access to the heap).
| /// `Py<T>` will hand out references to `Self::AsRefTarget`. This _must_ have the | ||
| /// same layout as `*mut ffi::PyObject`. |
There was a problem hiding this comment.
Isn't the layout part guaranteed by PyNativeType? Unless "this" refers to Self rather than the target.
There was a problem hiding this comment.
Yes, I think so. Probably still worth repeating this constraint here?
|
It looks like we don't need this / will remove it with #3382 so I'll close this. |
NB This is a breaking change, would be part of 0.19, not any 0.18.x patch release. Despite this, I would generally assume users don't ever refer to this directly, so I don't expect much impact downstream.
This PR moves
PyTypeInfo::AsRefTargetonto its own trait. There's a couple of reasons for this:PyIterator,PySequenceandPyMappingcan implement the new trait (they can't implementPyTypeInfo). This removes the need for them to provide special-cased methods.PyTypeInfowhich the experimental types cannot implement. So if we break this up, I think it will simplify that implementation.(Maybe we can wait until I push the rebooted #1308 proposal, to see whether we actually like it enough to try to adopt it.)