-
Notifications
You must be signed in to change notification settings - Fork 783
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
Pyerr isinstance #3826
Pyerr isinstance #3826
Conversation
8bef883
to
cb65379
Compare
CodSpeed Performance ReportMerging #3826 will degrade performances by 11.36%Comparing Summary
Benchmarks breakdown
|
cb65379
to
fb9af02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again! Got a few suggestions for this one...
src/err/mod.rs
Outdated
#[inline] | ||
pub fn is_instance(&self, py: Python<'_>, ty: &PyAny) -> bool { | ||
self.is_instance_bound(py, ty.into_py(py).into_bound(py)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With &Bound
for the argument, this should work:
self.is_instance_bound(py, ty.into_py(py).into_bound(py)) | |
self.is_instance_bound(py, &ty.as_borrowed()) |
Potentially are you willing to open a PR to recommend using as_borrowed()
as the way to go from &PyAny
to &Bound
in the migration guide? e.g.
let gil_ref: &PyAny = ...;
let bound: &Bound<PyAny> = &gil_ref.as_borrowed();
(hopefully most user code won't need to care about this because all of it will just get migrated over, but this might still be useful for users to be aware as they rewrite their code...)
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks again!
Implements more of the
Bound
api from #3684.