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

BytesMut::advance no longer advances cursor #725

Closed
camshaft opened this issue Aug 1, 2024 · 4 comments · Fixed by #726
Closed

BytesMut::advance no longer advances cursor #725

camshaft opened this issue Aug 1, 2024 · 4 comments · Fixed by #726

Comments

@camshaft
Copy link
Contributor

camshaft commented Aug 1, 2024

In #698, the behavior of BytesMut::advance was changed to clear the buffer instead of reduce capacity when the advanced count matched remaining. In s2n-quic, we rely on advance actually reducing remaining and have several data structures and trait impls that rely on this behavior:

In its current state, there's no longer an efficient way to actually reduce capacity of BytesMut, since the only other option is to call split_to and discard the result, which isn't great since you churn on reference counts, which is kind of the whole point of advance, as indicated by the #[must_use] attribute: consider BytesMut::advance if you don't need the other half.

My vote would be to revert this change.

@seanmonstar
Copy link
Member

Is there a simple test we could add with a comment to make sure we don't change this behavior again?

@camshaft
Copy link
Contributor Author

camshaft commented Aug 2, 2024

Is there a simple test we could add with a comment to make sure we don't change this behavior again?

I can open a PR tomorrow if no one beats me to it

@Darksonn
Copy link
Contributor

Darksonn commented Aug 2, 2024

Yes please, that would be great.

@camshaft
Copy link
Contributor Author

camshaft commented Aug 2, 2024

Opened #728

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 a pull request may close this issue.

3 participants