-
Notifications
You must be signed in to change notification settings - Fork 779
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 value bound #3820
Pyerr value bound #3820
Conversation
CodSpeed Performance ReportMerging #3820 will degrade performances by 15.02%Comparing Summary
Benchmarks breakdown
|
236caa6
to
1badb0e
Compare
1badb0e
to
ed51e80
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 for picking this one up!
Quite a few comments here, mostly related to removing .clone()
to avoid the reference counting cycles when we can, for efficiency wins 😄
13c9a72
to
6371ce1
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.
Whew, getting there, thanks for the iterations! This one has turned out to be quite fiddly.
I think the biggest thing remaining is whether from_value_bound
should just take Bound
instead of &Bound
; I think if you agree with that idea then it'll lead to a few more changes 🙈
6371ce1
to
e8e2f3d
Compare
This is easier to work with elsewhere than `&PyAny` or `Bound<'py, PyAny>`.
6520332
to
6cd9337
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.
I think we're there! There's just a few stylistic nits, and then let's merge this. Thank you for the several rounds of iteration on this one; turned out to have quite a few design points!
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.
🚀
Implements more of the
Bound
api from #3684.Follows on from #3819.