Replace more type casts with non-cast equivalents #437
Conversation
briansmith
force-pushed
the
b/casts-2
branch
4 times, most recently
from
58f3c01 to
c02f532
Compare
| @@ -19,7 +19,7 @@ pub fn getrandom_inner(mut dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | |||
| dest = &mut dest[res as usize..]; | |||
| } else { | |||
| let err = match res { | |||
| MIN_RET_CODE..=-1 => NonZeroU32::new(-res as u32).unwrap().into(), | |||
There was a problem hiding this comment.
Note that if res == -i32::MIN then -res overflows.
There was a problem hiding this comment.
MIN_RET_CODE is -i32::MAX, so -res can not overflow. The current one results in a slightly better codegen and prevents accidental creation of errors in the custom range (e.g. if we got a corrupted return code for some reason), but your code is a bit easier to read and does not contain any unwraps.
There was a problem hiding this comment.
prevents accidental creation of errors in the custom range
Good point. I will point out a couple of other places where this is an issue.
There was a problem hiding this comment.
This only relevant for Hermit, since it uses isize for return codes, while other targets use i32.
| @@ -13,6 +13,6 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | |||
| } else { | |||
| // ITRON error numbers are always negative, so we negate it so that it | |||
| // falls in the dedicated OS error range (1..INTERNAL_START). | |||
| Err(NonZeroU32::new((-ret) as u32).unwrap().into()) | |||
There was a problem hiding this comment.
unwrap here gets eliminated, but it still may be worth to use:
NonZeroU32::new(res.unsigned_abs()).unwrap_or(Error::UNEXPECTED)to quickly prevent any concerns about potential panics.
There was a problem hiding this comment.
Thanks. I was actually meaning to remove these unwrap()s in a separate PR, but since I did it in the hermit code, I also change it to do the same thing here.
briansmith
force-pushed
the
b/casts-2
branch
3 times, most recently
from
8f3322b to
1d82297
Compare
benches/buffer.rs
Outdated
| @@ -8,7 +8,7 @@ use std::mem::MaybeUninit; | |||
| fn bench_getrandom<const N: usize>() { | |||
| let mut buf = [0u8; N]; | |||
| getrandom::getrandom(&mut buf).unwrap(); | |||
| test::black_box(&buf as &[u8]); | |||
| test::black_box::<&[u8]>(&buf); | |||
There was a problem hiding this comment.
I think it's better to write it as test::black_box(&buf[..]).
| @@ -19,7 +19,7 @@ pub fn getrandom_inner(mut dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | |||
| dest = &mut dest[res as usize..]; | |||
| } else { | |||
| let err = match res { | |||
| MIN_RET_CODE..=-1 => NonZeroU32::new(-res as u32).unwrap().into(), | |||
There was a problem hiding this comment.
MIN_RET_CODE is -i32::MAX, so -res can not overflow. The current one results in a slightly better codegen and prevents accidental creation of errors in the custom range (e.g. if we got a corrupted return code for some reason), but your code is a bit easier to read and does not contain any unwraps.
| @@ -13,6 +13,6 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | |||
| } else { | |||
| // ITRON error numbers are always negative, so we negate it so that it | |||
| // falls in the dedicated OS error range (1..INTERNAL_START). | |||
| Err(NonZeroU32::new((-ret) as u32).unwrap().into()) | |||
There was a problem hiding this comment.
unwrap here gets eliminated, but it still may be worth to use:
NonZeroU32::new(res.unsigned_abs()).unwrap_or(Error::UNEXPECTED)to quickly prevent any concerns about potential panics.
(-x) overflows when x is the minimum value, where `x.unsigned_abs()` does the right thing. Avoid unreachable `unwrap()` in these conversions.
No description provided.