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

Rename and expose BytesMut::spare_capacity_mut #572

Merged
merged 1 commit into from Oct 4, 2022
Merged

Rename and expose BytesMut::spare_capacity_mut #572

merged 1 commit into from Oct 4, 2022

Conversation

yotamofek
Copy link
Contributor

This PR renames BytesMut::uninit_slice to spare_capacity_mut, to match Vec::spare_capacity_mut, and makes it a public API.

There are two reasons I'm proposing this change:

  • Even though BufMut::chunk_mut has a similar purpose, the fact that BufMut supports non-contiguous buffers makes unsafe code using it on a BytesMut type somewhat harder to reason about. The chunk_mut docs explicitly mention the returned slice might be less than buf.capacity(), which is untrue for BytesMut (because it is always contiguous), but not very clear when reading code using it.
  • <BytesMut as BufMut>::chunk_mut has additional logic to check whether the buffer is currently at full capacity, and reserve 64 additional bytes if it is. This is a redundant check when calling buf.reserve(n) followed by buf.chunk_mut(), but unfortunately the optimization process doesn't seem to be able to eliminate the unreachable branch. I've seen this to have, admittedly small, negative impact on performance in certain scenarios.

IMHO, spare_capacity_mut is a more descriptive name and I thought it would be helpful to match the std naming, which is why I renamed the method.
I've also aped the docs from the std docs for Vec, but slightly reworded them to fit BytesMut :)

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with adding this now that Vec::spare_capacity_mut is stabilized in the standard library, but I think the return type should be &mut [MaybeUninit<u8>] because it is not UB to de-initialize the data in the region.

@yotamofek
Copy link
Contributor Author

Thanks for the review, fixed!

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one doc comment.

src/bytes_mut.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@Darksonn Darksonn merged commit 6e4b1f2 into tokio-rs:master Oct 4, 2022
lelongg pushed a commit to lelongg/bytes that referenced this pull request Jan 9, 2023
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.

None yet

2 participants