Provide an easier-to-use rand API #1431
Conversation
src/rand.rs
Outdated
| assert!( | ||
| N == 0 || (isize::MAX as usize) / N >= mem::size_of::<T>(), | ||
| "Array too large to be converted to slice of u8" | ||
| ); |
There was a problem hiding this comment.
Is this assertion necessary? I thought it was already UB to have self be larger than isize bytes?
There was a problem hiding this comment.
We should have tests for the case where N == 0 and where core::mem::size_of<T> is zero.
There was a problem hiding this comment.
When I want to be sure the compiler can evaluate an expression at compile time, I do:
const NO_OVERFLOW: bool = N == 0 || (isize::MAX as usize) / N >= mem::size_of::<T>();
assert!(NO_OVERFLOW);
I'd rather not have the string in the assertion. I'm working to eliminate all such strings.
There was a problem hiding this comment.
Is this assertion necessary? I thought it was already UB to have
selfbe larger than isize bytes?
Ope I somehow managed to not read the * mem::size_of<T> part in the docs. I'll take that out - it makes matters simpler anyway.
| // derived from another mutable slice | ||
| // - Because T is POD, and because `len` is calculated based on the size of T, | ||
| // `data` points to `len` initialized u8s | ||
| // - We co-opt the lifetime of `self`, so no borrow checker rules are broken |
There was a problem hiding this comment.
I'm not sure about this:
// - We co-opt the lifetime of `self`, so no borrow checker rules are broken
It's hard for me to remember the exact circumstances, but I recall trying to do something similar and finding that I was running into a conflict with the mutable noalias optimization or similar.
This, in conjunction with wanting to avoid the need to initially zero all the bytes to start with, made me want to rewrite this whole module in terms of MaybeUninit and pointers.
WDYT?
There was a problem hiding this comment.
So I had not heard of the mutable noalias optimization before, and quick google searches of it suggest that it's actually buggy in the LLVM. In a world where there is no mis-compilation, the function signature should essentially be equivalent to fn as_mut_bytes<'a>(&'a mut self) -> &'a mut [u8], and since 'a will be inferred by the usage of the return value, it will extend the borrow of self accordingly. I can ask some other rustaceans I know about it to double check though.
When reading through the module I also thought about what it would look like to switch it over to using raw pointers. I think that's a viable option (and skips the pain of creating an initially zeroed-out array), but I didn't want to get waist deep with changing SecureRandom on my first contribution.
|
Is there any interest in moving the issue this PR addresses forward or should I close this PR? |
|
Thanks for the PR. See rust-random/getrandom#293 and the discussion in the related |
I saw that issue #728 hit a dead end a while ago, and I figured that with the somewhat recent stabilization of const generics that it could be picked up again. I extend random construction to arrays of all integer types, however I do use unsafe code to accomplish this. If it is desirable to stick with strictly safe code, then that is definitely possible, however it will likely be more inefficient.