Conversation
CodSpeed Performance ReportMerging #3770 will degrade performances by 16.93%Comparing 🎉 Hooray!
|
| Benchmark | main |
Icxolu:capsule |
Change | |
|---|---|---|---|---|
| ❌ | extract_float_downcast_success |
446.7 ns | 530 ns | -15.72% |
| ⚡ | not_a_list_via_downcast |
242.8 ns | 215 ns | +12.92% |
| ❌ | extract_float_extract_success |
408.9 ns | 492.2 ns | -16.93% |
|
Thank you so much for the continued help! I will do my best to find a moment to review this first thing in the morning tomorrow 🙏 |
adamreichold
left a comment
adamreichold
left a comment
There was a problem hiding this comment.
One suggestion on readability, otherwise LGTM.
src/types/capsule.rs
Outdated
| self.as_borrowed().name() | ||
| } | ||
|
|
||
| fn name_ptr_ignore_error(slf: &Bound<'_, Self>) -> *const c_char { |
There was a problem hiding this comment.
I think keeping this as an inherent method is a bit detrimental to readability. I would suggest making it a free function within this module and maybe putting it next to ensure_no_error.
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Agree with @adamreichold, otherwise just some tiny comments from me!
Thanks again 👍
src/types/capsule.rs
Outdated
| value: T, | ||
| name: Option<CString>, | ||
| ) -> PyResult<&Self> { | ||
| Self::new_bound(py, value, name).map(|b| b.into_gil_ref()) |
There was a problem hiding this comment.
I slightly prefer the following style for the map:
| Self::new_bound(py, value, name).map(|b| b.into_gil_ref()) | |
| Self::new_bound(py, value, name).map(Bound::into_gil_ref) |
src/types/capsule.rs
Outdated
| name: Option<CString>, | ||
| destructor: F, | ||
| ) -> PyResult<&'_ Self> { | ||
| Self::new_bound_with_destructor(py, value, name, destructor).map(|b| b.into_gil_ref()) |
There was a problem hiding this comment.
Ditto here
| Self::new_bound_with_destructor(py, value, name, destructor).map(|b| b.into_gil_ref()) | |
| Self::new_bound_with_destructor(py, value, name, destructor).map(Bound::into_gil_ref) |
| let cap_ptr = ffi::PyCapsule_New( | ||
| Box::into_raw(val) as *mut c_void, | ||
| ffi::PyCapsule_New( | ||
| Box::into_raw(val).cast(), |
|
Thanks for the review, I've adjusted the points. |
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks yet again, looks great!
3 participants
This ports
PyCapsuleto the new Bound API from #3684.