Conversation
Also assert that pointer width is an expected value.
newpavlov
left a comment
There was a problem hiding this comment.
It may be worth to add a build-only CI job for a 16-bit target to prevent regressions in future.
|
|
||
| use rand_core::{RngCore, SeedableRng}; | ||
|
|
||
| #[cfg(any(target_pointer_width = "32", target_pointer_width = "16"))] |
There was a problem hiding this comment.
Why change it from not(target_pointer_width = "64")? Even with hypothetical 128-bit targets it's better to use Xoshiro128++ than to break builds. Alternatively, it's worth to at least add a compile_error! stating that we support only 16, 32, and 64 bit targets.
There was a problem hiding this comment.
- Because this is explicit about what is supported and recommended
- Effectively it is a compile error
- I don't care about theoretical targets. If people want to target 16- or 128-bits on rand, they can submit patches.
src/distributions/slice.rs
Outdated
| use alloc::string::String; | ||
|
|
||
| #[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))] | ||
| const _: () = assert!(false, "unsupported pointer width"); |
There was a problem hiding this comment.
Why not use compile_error! here? Also, why can't we use UniformSize::U32 on 16-bit targets?
There was a problem hiding this comment.
- I forgot about it — I'll switch to
compile_error! - Dependencies don't compile and it seems like no-one cares much about 16-bit (Supported target_pointer_width: 32, 64-bit only? #1468), so there's no point wasting time here.
| enum UniformUsize { | ||
| U32(Uniform<u32>), | ||
| #[cfg(target_pointer_width = "64")] | ||
| U64(Uniform<u64>), | ||
| } |
There was a problem hiding this comment.
As with gen_index, this should probably be exposed somehow (left for a future PR).
- Fix portability of `choose_multiple_array` - Fix portability of `rand::distributions::Slice`
CHANGELOG.mdentrySummary
Fixes some of the smaller issues noted here. Specifically:
choose_multiple_arrayrand::distributions::SliceFurther, this makes it more explicit which
target_pointer_widthsizesSmallRngbackends are intended to support (the 16-bit choice is not intended to be optimal).