Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Buf::copy_to_bytes #129

Closed
stepancheg opened this issue May 25, 2017 · 2 comments · Fixed by #439
Closed

Buf::copy_to_bytes #129

stepancheg opened this issue May 25, 2017 · 2 comments · Fixed by #439
Assignees
@seanmonstar
Milestone
v0.6

Comments

@stepancheg
Copy link
Contributor

Buf could have copy_to_bytes operation:

fn copy_to_bytes(&mut self, len: usize) -> Bytes;

Default implementation should simply allocate new Bytes, while implementation for Bytes could call slice.
@vincentdephily
Copy link

I'd like this too. There's Buf::to_bytes() which consumes the remainder of the buffer, but that's rarely what we want. We need a get_bytes() like we need the other get_*().

vincentdephily added a commit to vincentdephily/mqttrs that referenced this issue Jan 10, 2020
@vincentdephily
Partial switch from BytesMut to Buf/BufMut.
85bd2e3 
The initial idea was to have `decode()` take a `Buf` and `encode()` take a `ButMut`, as they
semantically should. It would have enabled our users to use other buffer backends.

Switching `encode()` to take a BufMut was straightforward, that's done.

Concerning `decode()`, `Buf` currently lacks facilities to rewind the read position
(tokio-rs/bytes#330) and to read a specified number of bytes
(tokio-rs/bytes#129), making it impossible to implement the desired API
unless we assume that our user is using a continuous-memory backend like BytesMut (and even then,
the implementation isn't really pretty).

It is possible to have `decode()` take a `Bytes` (as it semantically should) instead of a
`BytesMut`, but because users typically want to pass a `BytesMut` to `TcpStrem.read()` it would
require a `.freeze()` before passing to `decode()`, which is cumbersome and prevents buffer reuse.

With all that said, this commit does move things in the right direction, and if upstream fixes the
mentioned bugs it'll be that much easyer to switch.
vincentdephily added a commit to vincentdephily/mqttrs that referenced this issue Jan 10, 2020
@vincentdephily
Partial switch from BytesMut to Buf/BufMut.
71627f9 
The initial idea was to have `decode()` take a `Buf` and `encode()` take a `ButMut`, as they
semantically should. It would have enabled our users to use other buffer backends.

Switching `encode()` to take a BufMut was straightforward, that's done.

Concerning `decode()`, `Buf` currently lacks facilities to rewind the read position
(tokio-rs/bytes#330) and to read a specified number of bytes
(tokio-rs/bytes#129), making it impossible to implement the desired API
unless we assume that our user is using a continuous-memory backend like BytesMut (and even then,
the implementation isn't really pretty).

It is possible to have `decode()` take a `Bytes` (as it semantically should) instead of a
`BytesMut`, but because users typically want to pass a `BytesMut` to `TcpStrem.read()` it would
require a `.freeze()` before passing to `decode()`, which is cumbersome and prevents buffer reuse.

With all that said, this commit does move things in the right direction, and if upstream fixes the
mentioned bugs it'll be that much easyer to switch.
vincentdephily added a commit to vincentdephily/mqttrs that referenced this issue Mar 3, 2020
@vincentdephily
Partial switch from BytesMut to Buf/BufMut.
5802538 
The initial idea was to have `decode()` take a `Buf` and `encode()` take a `ButMut`, as they
semantically should. It would have enabled our users to use other buffer backends.

Switching `encode()` to take a BufMut was straightforward, that's done. This solves a gotcha about
using `bytes::BytesMut` vs `bytes::bytes_mut::BytesMut`, and enables the use of other buffer types,
like `Vec`.

Concerning `decode()`, `Buf` currently lacks facilities to rewind the read position
(tokio-rs/bytes#330) and to read a specified number of bytes
(tokio-rs/bytes#129), making it impossible to implement the desired API
unless we assume that our user is using a continuous-memory backend like BytesMut (and even then,
the implementation isn't really pretty).

It is possible to have `decode()` take a `Bytes` (as it semantically should) instead of a
`BytesMut`, but because users typically want to pass a `BytesMut` to `TcpStrem.read()` it would
require a `.freeze()` before passing to `decode()`, which is cumbersome and prevents buffer reuse.

With all that said, this commit does move things in the right direction, and if upstream fixes the
mentioned bugs it'll be that much easyer to switch.
@mzabaluev mzabaluev mentioned this issue Jun 16, 2020
Closed
@mzabaluev
Copy link
Contributor

mzabaluev commented Jun 16, 2020 •
edited
Loading

Bikeshedding: copy_to_bytes does not sound true to what this method does (as far as I understand). Taking bytes from under self should be emphasized, and maybe the potential for zero-copy implementation.

Maybe extract_to_bytes? I've got some prior art, though not as an intrinsic method: StrChunk::extract_utf8

@carllerche carllerche added this to the v0.6 milestone Oct 16, 2020
carllerche added a commit that referenced this issue Oct 20, 2020
@carllerche
Add Buf::copy_to_bytes(len)
c1dcec3 
This method replaces `Buf::to_bytes()`, providing a method that copies a
subset of the remaining buffer into a `Bytes` value. As this is strictly
more flexible, `to_bytes()` is removed.

Fixes: #129, #398
carllerche added a commit that referenced this issue Oct 20, 2020
@carllerche
Add Buf::copy_to_bytes(len)
9de615f 
This method replaces `Buf::to_bytes()`, providing a method that copies a
subset of the remaining buffer into a `Bytes` value. As this is strictly
more flexible, `to_bytes()` is removed.

Fixes: #129, #398
@carllerche carllerche mentioned this issue Oct 20, 2020
Merged
carllerche added a commit that referenced this issue Oct 20, 2020
@carllerche
Add Buf::copy_to_bytes(len) (#439)
39de065 
This method replaces `Buf::to_bytes()`, providing a method that copies a
subset of the remaining buffer into a `Bytes` value. As this is strictly
more flexible, `to_bytes()` is removed.

Fixes: #129, #398
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@seanmonstar seanmonstar

Labels
None yet
Projects
None yet
Milestone
v0.6
Development

Successfully merging a pull request may close this issue.

Add Buf::copy_to_bytes(len) tokio-rs/bytes
5 participants
@carllerche @stepancheg @seanmonstar @vincentdephily @mzabaluev