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.

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

Implement Bytes::unsplit #538

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adeschamps
Copy link
Contributor

This is based largely on the example of BytesMut::unsplit. If two
Bytes are contiguous and point to the same allocation, then they are
cheaply merged. Otherwise, a new BytesMut is allocated, copies data
from self and other, and self is replaced with new.freeze().

Closes #503

This is based largely on the example of `BytesMut::unsplit`. If two
`Bytes` are contiguous and point to the same allocation, then they are
cheaply merged. Otherwise, a new `BytesMut` is allocated, copies data
from `self` and `other`, and `self` is replaced with `new.freeze()`.

Closes tokio-rs#503
@SephVelut
Copy link

SephVelut commented Mar 30, 2023

try_unsplit only has one fail case that's side-effect free. It doesn't pose a danger to public users. Why not make it pub?

There's two use cases where I will want to merge Bytes

  1. merge two non-contiguous Bytes to have a convenient single view
  2. merge two contiguous Bytes that have previously been split off

In the first case, if the Bytes happen to be contiguous, then the O(1) operation is just a bonus. But in the second case, I expect that the Bytes must be contiguous. Otherwise it would be an error to merge them. unsplit hides this failure and is general over both cases. Maybe rename unsplit to merge and keep as is, but rename try_unsplit to unsplit and keep as is and expose both.

@SephVelut
Copy link

When using current unsplit, it's not immediately obvious to a pub user that the operation will soft-fail if the owning BytesMut reallocates because of insufficient capacity. In hindsight this is obvious, but this method gives off the impression that Bytes simply need to be contiguous. Emphasis on both contiguous and from the same allocation with a note in the doc that a reallocation will invalidate formerly contiguous blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider an unsplit method for Bytes
2 participants