Conversation
CodSpeed Performance ReportMerging #3777 will degrade performances by 15.02%Comparing Summary
Benchmarks breakdown
|
davidhewitt
force-pushed
the
bytes-new-bound
branch
from
d32bacd to
01c97c7
Compare
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
Left some small suggestions above.
Also, do we want implement to Index for Bound/&Bound as well, or do we want to require calling as_bytes() before indexing?
Otherwise LGTM
pytests/src/buf_and_str.rs
Outdated
| fn return_memoryview(py: Python<'_>) -> PyResult<&PyMemoryView> { | ||
| let bytes: &PyAny = PyBytes::new(py, b"hello world").into(); | ||
| let bytes: &PyAny = PyBytes::new_bound(py, b"hello world").into_gil_ref(); |
There was a problem hiding this comment.
after #3786 this could return a Bound<PyMemoryView> now 👼
| @@ -80,10 +113,23 @@ impl PyBytes { | |||
| /// `std::slice::from_raw_parts`, this is | |||
| /// unsafe](https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html#safety). | |||
| pub unsafe fn from_ptr(py: Python<'_>, ptr: *const u8, len: usize) -> &PyBytes { | |||
There was a problem hiding this comment.
Since this returns a gil-ref, should we deprecate it as well, in favor of bound_from_ptr()?
There was a problem hiding this comment.
Yes, my mistake to miss this; that's the downside of the commits I've been cherry-picking from #3606 - they were mostly trying to get a complete compile with a good range of the types rather than a complete implementation 🙈
I'll add here 👍
|
Thanks for the review!
Ah great spot - unfortunately |
Oh actually looking again here it is possible in this case, because we can still return |
davidhewitt
force-pushed
the
bytes-new-bound
branch
from
01c97c7 to
f7a920b
Compare
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
Oh actually looking again here it is possible in this case, because we can still return &u8! Even better that you made me check 👍
Great, I could not see the reason why this would not work here. Glad I'm not blind 😆
| } | ||
|
|
||
| /// This is the same way [Vec] is indexed. | ||
| impl<I: SliceIndex<[u8]>> Index<I> for Bound<'_, PyBytes> { |
There was a problem hiding this comment.
we could additionally implement it on &Bound as well, so it also works an a borrowed bound
There was a problem hiding this comment.
I think it's not needed due to the way bytes[i] gets desugared into something like *bytes.index(i).
I've pushed an additional part to test_bound_bytes_index which indexes on &Bound<PyBytes>, see if you agree with me that the one implementation is sufficient?
There was a problem hiding this comment.
Perfect, that even better! 👍
davidhewitt
force-pushed
the
bytes-new-bound
branch
from
f7a920b to
d41412c
Compare
davidhewitt
force-pushed
the
bytes-new-bound
branch
from
d41412c to
4c94be5
Compare
|
Thanks both! 🙏 |
3 participants
For #3684
This adds
PyBytes::new_boundandPyBytes::new_bound_with, deprecating the old constructors and migrating internal code.