Add conversion from Bytes to Vec#151
Conversation
|
Public |
|
I quite like the pattern of having a generic version and a monomorphic function that covers the most common case in order to make type inference nicer, but it's your call. |
|
I think
Anyway, it's not exactly my call, because I'm not the maintainer of the library. |
|
Oh sorry, I got you confused with the maintainer. Yeah, I agree with that thinking. I think the best thing to do here is to keep the function but make it private, since I prefer unsafe code to be contained in inherent impls if possible. I'll wait to see if the maintainer responds though. |
|
I'm fine w/ adding a conversion -> Vec, but yeah it can't be guaranteed to be cheap in all cases. |
|
I guess you're right that it gives the wrong impression, I'll drop the |
This also make `into_vec` methods private and correspondingly fixes up the doctests for `Bytes::into_vec`.
eira-fransham
force-pushed
the
rebase-into-vec
branch
from
464f8f4 to
824a986
Compare
|
@carllerche Any update on this? |
| return r; | ||
| } | ||
|
|
||
| if kind == KIND_ARC { |
There was a problem hiding this comment.
If I'm reading this correctly, this function assumes that a BytesMut handle has a unique Arc<Shared> (no other clones). However, that is not the case. There can be many outstanding handles to the same Arc<Shared>. BytesMut only guarantees that it has exclusive access to a window of the shared buffer. So, it can mutate the bytes in its window, but it does not have exclusive ownership of the buffer.
My guess is this function should be deleted in favor of just always calling Inner2::into_vec.
| /// // memory is not allocated for the result, it is the same object | ||
| /// assert_eq!(ptr, vec.as_slice().as_ptr()); | ||
| /// ``` | ||
| fn into_vec(self) -> Vec<u8> { |
There was a problem hiding this comment.
I am OK w/ making this public. It's fine to have inherent struct functions as well as From implementations.
| /// // memory is not allocated for the result, it is the same object | ||
| /// assert_eq!(ptr, vec.as_slice().as_ptr()); | ||
| /// ``` | ||
| fn into_vec(self) -> Vec<u8> { |
There was a problem hiding this comment.
I am OK w/ making this public. It's fine to have inherent struct functions as well as From implementations.
| } | ||
|
|
||
| if kind == KIND_ARC { | ||
| let arc = self.arc.load(Relaxed); |
There was a problem hiding this comment.
Could you add a comment stating that this Relaxed load is safe as we are guaranteed to have exclusive access to the atomic by virtue of the function taking self.
|
Thanks for the PR. I added some inline comments. |
|
Relates to #86 |
|
What's progress of this PR? I think it's really useful for performance critical code. @carllerche |
4 participants
This takes the
into_vecfunction from this PR and makes it work on master. It also extends it with an equivalentBytesMutversion and aInto<Vec<u8>>impl for each.