Fix test_uint32_shr failing on debug builds.#98
Conversation
| for _ in 0..50 { | ||
| for i in 32..60 { | ||
| let num = rng.gen(); | ||
| let result = std::panic::catch_unwind(|| UInt32::constant(num).shr(i)); |
There was a problem hiding this comment.
I think a better approach is using #[should_panic] on the test itself.
There was a problem hiding this comment.
I couldn't do that because I wanted the test to make sure it panics on a bunch of different values, whereas #[should_panic] would only ensure one of the values cause a panic.
Simplify dependencies
defuse
left a comment
defuse
left a comment
There was a problem hiding this comment.
Oops! I thought I replied to the comment but it never got posted because my reply was part of this review which I never submitted!
| for _ in 0..50 { | ||
| for i in 32..60 { | ||
| let num = rng.gen(); | ||
| let result = std::panic::catch_unwind(|| UInt32::constant(num).shr(i)); |
There was a problem hiding this comment.
I couldn't do that because I wanted the test to make sure it panics on a bunch of different values, whereas #[should_panic] would only ensure one of the values cause a panic.
daira
left a comment
daira
left a comment
There was a problem hiding this comment.
utACK with nonblocking comment.
| fn test_uint32_shr_overflow() { | ||
| let mut rng = XorShiftRng::from_seed([0x5dbe6259, 0x8d313d76, 0x3237db17, 0xe5bc0654]); | ||
|
|
||
| for _ in 0..50 { |
There was a problem hiding this comment.
Is it really needed to do this 50 times? The behaviour is easily seen from the source to not be value-dependent.
|
This PR would need to be migrated over to the main rust crates. |
4 participants
This makes
shrbehave differently than Rust's>>in release mode but IMO how>>works in release mode is kinda dumb -- I could see myself assuming the argument to shr or the right argument to>>saturates instead of taking the value mod 32, and introducing security bugs that way -- so this seems safer.