bytes icon indicating copy to clipboard operation
bytes copied to clipboard

Consider adding `Buf::into_bytes_mut()`

Open carllerche opened this issue 5 years ago • 2 comments

Motivation

The Buf trait is designed to provide an abstraction for reading data about of byte slice like types (&[u8], Vec<u8>, ropes, etc...).

A difficulty arises when one wishes to perform more complex operations with a data slice. For example, any sort of in-place data mutation.

In order to mutate the contents of a Buf, the Buf must be read into a storage type (such as Vec<u8> or BytesMut). The problem is that the Buf type might already be in an appropriate storage type.

Proposal

I propose to add the following to the Buf trait:

fn into_bytes_mut(self) -> BytesMut where Self: Sized {
    let mut ret = BytesMut::with_capacity(self.remaining());
    ret.put(self)
    ret
}

This would be a backwards compatible. It would also let types that are able to have improved implementations. For example:

impl Buf for BytesMut {
    fn into_bytes_mut(self) -> BytesMut where Self: Sized {
        self
    }
}

Although, in 0.4, Buf is not implemented for BytesMut, this will work in 0.5. That said, this change would still help today as this change is motivated by BufStream. An implementation of BufStream for BytesMut could use a (currently undefined) BytesBuf type that impls Buf and has an efficient into_bytes_mut impl.

carllerche avatar Sep 06 '18 19:09 carllerche

The motivation makes sense, the only thing that feels off is that it ties BytesMut more directly into Buf, which feels like some sort of polution... I know they are in the same crate right now, but we've talked before about if Buf/BufMut should be in a base trait, so that they aren't tied to breaking changes of Bytes/BytesMut.

seanmonstar avatar Sep 06 '18 20:09 seanmonstar

I was thinking about it more, I am currently of the mind that they should stay together.

First, there will doubtfully be many more breaking changes after this. We've been using it for a while and the only real issue has been the IntoBuf implementation. Second, this use case would require them to stay together.

carllerche avatar Sep 06 '18 20:09 carllerche