Move rebuild_vec into a method #659
Conversation
The rebuild_vec function is always called with the same first three arguments: the pointer, length, and capacity of a particular BytesMut instance. Instead of passing all of those each time, we can use a method on BytesMut.
src/bytes_mut.rs
Outdated
| /// Rebuild a vec from `offset` with the existing pointer, length, and capacity. | ||
| #[inline] | ||
| unsafe fn rebuild_vec(&self, offset: usize) -> Vec<u8> { |
There was a problem hiding this comment.
Can you document a bit about why this is unsafe? That is, explain that this transfers ownership of the allocation to the vector, but that it doesn't remove the pointers from the BytesMut that it is called on. (And of course, the offset must be correct.)
I know that it didn't explain this before your change, but by moving it to BytesMut, you're introducing the connotation that it could modify the BytesMut to no longer own the allocation. That connotation was not there when it was a standalone method.
There was a problem hiding this comment.
Thanks for pointing that out @Darksonn! I added some information in f62a4d0. Let me know if you prefer something different.
Also, the source of offset is always the offset from self.get_vec_pos, so we could probably remove that requirement by fetching it in this method. I'll attempt that separately though!
| let shared = Box::new(Shared { | ||
| vec: rebuild_vec(self.ptr.as_ptr(), self.len, self.cap, off), | ||
| vec: self.rebuild_vec(off), | ||
| original_capacity_repr, | ||
| ref_count: AtomicUsize::new(ref_cnt), | ||
| }); |
There was a problem hiding this comment.
And you don't actually follow the safety requirements as stated here.
|
You're right - I prefer safety comments like that too and I definitely didn't follow them here. I think this is because Thank you for your thoughts @Darksonn! |
The rebuild_vec function is always called with the same first three arguments: the pointer, length, and capacity of a particular BytesMut instance. Instead of passing all of those each time, we can use a method on BytesMut. Sound good?