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

Add conversion from BytesMut to Vec<u8> #543

Merged
merged 6 commits into from Jul 10, 2022
Merged

Add conversion from BytesMut to Vec<u8> #543

merged 6 commits into from Jul 10, 2022

Conversation

NobodyXu
Copy link
Contributor

Fixed #427

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

@Darksonn
Copy link
Contributor

I'm not a super big fan of using a TryInto impl for this since the conversion is never really fallible - it's just that sometimes it isn't free.

I also wonder about your ptr::copy, since if we are to provide a conversion that fails if the operation is not free, then surely it should also fail if copying the data is necessary?

@NobodyXu
Copy link
Contributor Author

I'm not a super big fan of using a TryInto impl for this since the conversion is never really fallible - it's just that sometimes it isn't free.

I also wonder about your ptr::copy, since if we are to provide a conversion that fails if the operation is not free, then surely it should also fail if copying the data is necessary?

You are right, I should probably just implement a Into<Vec<u8>> for BytesMut.

@NobodyXu
Copy link
Contributor Author

@Darksonn I have replaced the TryInto implementation with Into implementation.

@NobodyXu
Copy link
Contributor Author

Pinging this PR

@NobodyXu NobodyXu mentioned this pull request May 1, 2022
Merged
@joshtriplett
Copy link

I'd love to have this as well, to avoid having to open-code it in multiple places.

c410-f3r
c410-f3r reviewed Jul 6, 2022
View reviewed changes
src/bytes_mut.rs Outdated Show resolved Hide resolved
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jul 6, 2022

CI/miri failed due to strict provenance being enabled and the bytes codebase uses integer-pointer conversions and vice versa.

I also run miri locally with strict provenance and it succeeded with warning on integer-pointer cast.

This PR itself does not add any integer-pointer conversion, so it should not block this PR from being merged.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2022

Master has a commit that makes bytes compatible with strict provenance, but your PR doesn't appear to have it. Can you rebase/merge in master?

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jul 6, 2022

@Darksonn I've just rebased this PR today.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2022

You must have forgotten to pull the changes from our master before rebasing, because your PR's branch does not have any changes newer than April 15.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jul 6, 2022

@Darksonn But my master branch is up-to-date.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2022

Oh. I see what's going on. One of our representations of BytesMut stores integer data in a pointer field. It's never actually used as a pointer - it's cast back to an integer before being used, but the strict provenance check still complains about it. I'm guessing that this behavior was changed in a newer version of miri.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2022

Please see #553.

@NobodyXu
Impl TryInto<Vec<u8>> for BytesMut
ae48800 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Replace TryInto<Vec<u8>> with Into<Vec<u8>>
c5b8f39 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix Into<Vec<u8>> impl for BytesMut
aad93ea 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Increase coverage of bytes_into_vec
0f4ab70 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix Into<Vec<u8>> impl for BytesMut
5dfc926 
Ensure that if new kind/vtable is added to `BytesMut` in the future,
`Into<Vec<u8>>` would still work.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Replace Into<Vec<u8>> with From<BytesMut>
d3c8b8d 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Copy link
Contributor Author

@Darksonn I have rebased this PR.

Darksonn
Darksonn approved these changes Jul 10, 2022
View reviewed changes
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 to me.

@Darksonn Darksonn merged commit 068ed41 into tokio-rs:master Jul 10, 2022
@NobodyXu NobodyXu deleted the feature/into-vec branch July 10, 2022 12:25
@Darksonn Darksonn mentioned this pull request Jul 19, 2022
Merged
lelongg pushed a commit to lelongg/bytes that referenced this pull request Jan 9, 2023
@NobodyXu @lelongg
Add conversion from BytesMut to Vec<u8> (tokio-rs#543)
30015c2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c410-f3r c410-f3r c410-f3r left review comments

@Darksonn Darksonn Darksonn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Conversion from Bytes to Vec<u8>?
4 participants
@NobodyXu @Darksonn @joshtriplett @c410-f3r