add bound method variants for PyTypeInfo
#3782
Conversation
| fn type_object(py: Python<'_>) -> &PyType { | ||
| unsafe { py.from_borrowed_ptr(Self::type_object_raw(py) as _) } | ||
| } | ||
|
|
||
| /// Returns the safe abstraction over the type object. | ||
| #[inline] | ||
| fn type_object_bound(py: Python<'_>) -> Bound<'_, PyType> { |
There was a problem hiding this comment.
I considered whether to make type_object_bound, is_type_of_bound and is_exact_type_of_bound have defaults implemented in terms of their original methods (or vice versa).
Given that we don't expect users to be implementing PyTypeInfo, I decided it was probably simpler to just give them all defaults which match their final expected forms.
There was a problem hiding this comment.
I agree that the new methods should be implemented in their final form. Whats the reason not reimplementing the old API in terms of the new one?
There was a problem hiding this comment.
That's a very good point, I will go ahead and do that 👍
There was a problem hiding this comment.
Ah, there is one reason: type_object_bound now returns an owned Bound instead of a borrowed gil-ref, which I think is correct for the safety reasoning I express in the note below.
But is_type_of and is_exact_type_of can be implemented in terms of their new replacements 👍
davidhewitt
force-pushed
the
type-check-bound
branch
from
1cc422b
to
67ace70
Compare
CodSpeed Performance ReportMerging #3782 will degrade performances by 22.87%Comparing 🎉 Hooray!
|
| Benchmark | main |
davidhewitt:type-check-bound |
Change | |
|---|---|---|---|---|
| ⚡ | not_a_list_via_downcast |
272.2 ns | 216.7 ns | +25.64% |
| ❌ | sequence_from_tuple |
232.8 ns | 260.6 ns | -10.66% |
| ❌ | list_via_extract |
281.1 ns | 364.4 ns | -22.87% |
| ⚡ | sequence_from_list |
300 ns | 272.2 ns | +10.2% |
| ❌ | list_via_downcast |
157.2 ns | 185 ns | -15.02% |
davidhewitt
force-pushed
the
type-check-bound
branch
from
67ace70
to
669aab7
Compare
| fn type_object(py: Python<'_>) -> &PyType { | ||
| unsafe { py.from_borrowed_ptr(Self::type_object_raw(py) as _) } | ||
| } | ||
|
|
||
| /// Returns the safe abstraction over the type object. | ||
| #[inline] | ||
| fn type_object_bound(py: Python<'_>) -> Bound<'_, PyType> { |
There was a problem hiding this comment.
I agree that the new methods should be implemented in their final form. Whats the reason not reimplementing the old API in terms of the new one?
| fn is_type_of(object: &PyAny) -> bool { | ||
| unsafe { ffi::PyObject_TypeCheck(object.as_ptr(), Self::type_object_raw(object.py())) != 0 } | ||
| } | ||
|
|
||
| /// Checks if `object` is an instance of this type or a subclass of this type. | ||
| #[inline] | ||
| fn is_type_of_bound(object: &Bound<'_, PyAny>) -> bool { |
There was a problem hiding this comment.
is_bound_type_of might read a little nicer (maybe, not sure 🙃, maybe someone else has an opinion too ). Similarly for the others
There was a problem hiding this comment.
Hmm, I'm a bit stuck on that too. I don't really like either option, so my thinking was to stick with _bound as a suffix because that matches the majority of the other APIs...
There was a problem hiding this comment.
I tend to agree, they both read a bit odd, so its probably better to stick with the system we've introduced
davidhewitt
force-pushed
the
type-check-bound
branch
from
669aab7
to
367eeae
Compare
|
Thanks for the review! I've given this a rebase and actioned all the feedback at the same time. |
| fn is_type_of(object: &PyAny) -> bool { | ||
| unsafe { ffi::PyObject_TypeCheck(object.as_ptr(), Self::type_object_raw(object.py())) != 0 } | ||
| } | ||
|
|
||
| /// Checks if `object` is an instance of this type or a subclass of this type. | ||
| #[inline] | ||
| fn is_type_of_bound(object: &Bound<'_, PyAny>) -> bool { |
There was a problem hiding this comment.
I tend to agree, they both read a bit odd, so its probably better to stick with the system we've introduced
Part of #3684
This PR updates the
PyTypeInfoandPyTypeChecktraits for theBoundAPI:PyTypeCheckis new in 0.21 so I immediately made it take&Bound<'_, PyAny>without any backwards compatibilityPyTypeInfohas had new_boundmethod variants added for those which took or returned gil-refs. I added the usual deprecation warnings and migrated internal code to these new methods. (That makes up a fair chunk of this diff.)