Skip to content

Add a function to create headers from byte arrays without any transmute #143

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

Merged
merged 3 commits into from
Dec 31, 2017

Conversation

est31
Copy link
Contributor

@est31 est31 commented Dec 29, 2017

No description provided.

@est31
Copy link
Contributor Author

est31 commented Dec 29, 2017

After merging of this PR, I'd love to have a (minor) release.

@est31
Copy link
Contributor Author

est31 commented Dec 29, 2017

It's not really important though as I can use a transmute for now.

@alexcrichton
Copy link
Owner

This is a pretty large argument to be passing around, perhaps this could be switched to take a slice? We could also have it take a variable-length slice, assert it's 512 bytes, and then return a reference.

@est31
Copy link
Contributor Author

est31 commented Dec 30, 2017

@alexcrichton is it better now? I'll need to test this version of the function first whether it actually works...

@alexcrichton
Copy link
Owner

Yeah that looks fine by me, although could this take &[u8] instead of &[u8; 512]? I imagine the latter is very difficult to construct...

@est31
Copy link
Contributor Author

est31 commented Dec 31, 2017

I imagine the latter is very difficult to construct...

Indeed... My code can manage as it serializes from/to the array directly, but I'll change it to &[u8]. As slices also need to store the length, I'll have to convert the slice to a pointer.

@est31
Copy link
Contributor Author

est31 commented Dec 31, 2017

Please don't merge this yet, I haven't verified whether it does what I want it to. With unsafe code you never know...

@est31
Copy link
Contributor Author

est31 commented Dec 31, 2017

Hmm right there seems to be an issue here where we invoke UB... Let me see whether I can fix this.

@est31
Copy link
Contributor Author

est31 commented Dec 31, 2017

Yup, it is fixed now. r? @alexcrichton

@alexcrichton alexcrichton merged commit dc4aff7 into alexcrichton:master Dec 31, 2017
@alexcrichton
Copy link
Owner

Thanks!

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.

2 participants