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

Make Fill_via_u*_chunks not modify src #242

Merged
merged 2 commits into from
Jan 21, 2018

Conversation

pitdicker
Copy link
Contributor

I now have 4 branches with BlockRngCore trait experiments, but it is hard to get them in a reviewable state. It is mostly the addition of a BlockRng wrapper and just a lot of code moving around.

One functional change is the one in this PR. Because it touches somewhat difficult code using unsafe it seemed best to split it out.

Another thing I can split out are changes to the documentation of ChaCha.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Looks mostly good, though it's not the first time I've been unimpressed by byteorder code.

src/impls.rs Outdated
unsafe {
// N.B. https://github.com/rust-lang/rust/issues/22776
let bytes = transmute::<_, [u8; $size]>(n.to_le());
copy_nonoverlapping((&bytes).as_ptr(),
Copy link
Member

Choose a reason for hiding this comment

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

bytes is an array, so bytes.as_ptr() should be the same? I'd prefer the following, though unfortunately it means passing another parameter (or get size from size_of):

let tmp = n.to_le();
let src_ptr = &tmp as *const $ty as *const [u8; $size];  // inline this is you like

I don't think you need to reference that bug; I think Rust's lifetime analysis is good enough now to prevent it.

@pitdicker
Copy link
Contributor Author

Thank you, your alternative looks better 😄.

@dhardy
Copy link
Member

dhardy commented Jan 21, 2018

Glad it worked out. Now we have 4 PRs ready except for CI; hmm...

@pitdicker
Copy link
Contributor Author

Yes... All these changes are platform-independent. If the CI passes on Linux, would it be ok to merge?

@dhardy dhardy merged commit 97d0a46 into rust-random:master Jan 21, 2018
@pitdicker pitdicker deleted the fill_via_chunks_immutable branch January 21, 2018 15:15
@dhardy
Copy link
Member

dhardy commented Jan 21, 2018

It's big-endian tests that are really missed. Maybe we do need to port your Travis tweaks to this branch.

@pitdicker
Copy link
Contributor Author

I also missed them for the ChaCha PR... Maybe it is best to use just one big-endian platform from it.

pitdicker pushed a commit that referenced this pull request Apr 4, 2018
Make `Fill_via_u*_chunks` not modify `src`
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