Increase MSRV to 1.38 and use ptr.cast::<T>() whenever practical to clarify casts.
#425
Conversation
briansmith
force-pushed
the
b/cast-1
branch
2 times, most recently
from
9b31049 to
334178c
Compare
|
This change would require bumping the MSRV from 1.36 to 1.38, which I think is worthwhile. |
|
As noted here, we are likely to release v0.3 somewhere in the near future, so I think this change can wait until then. |
briansmith
changed the title
ptr.cast::<T>() whenever practical to clarify casts.ptr.cast::<T>() whenever practical to clarify casts.
| @@ -14,7 +14,7 @@ extern "C" { | |||
| } | |||
|
|
|||
| pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | |||
| let ret = unsafe { CCRandomGenerateBytes(dest.as_mut_ptr() as *mut c_void, dest.len()) }; | |||
| let ret = unsafe { CCRandomGenerateBytes(dest.as_mut_ptr().cast::<c_void>(), dest.len()) }; | |||
There was a problem hiding this comment.
I think we can rely on type inference and use just .cast().
There was a problem hiding this comment.
I intentionally avoid using type inference so that we ensure we're casting to the expected type. That is one of the goals of this change.
There was a problem hiding this comment.
I don't think it helps in any practical way in this case. In the first place, pointer types do not matter on the ABI level, so we are doing casting to just satisfy conventional signature of external functions. Also, IIRC std also uses a lot of pointer casts which rely on type inference in cases like this.
There was a problem hiding this comment.
It doesn't seem to help in our code because our code does seem to be correct. But this style of explicitly naming the type has helped prevent bugs in practice in other code, and it makes the code easier to audit (from my own experience).
There was a problem hiding this comment.
I personally like having the type explicit in the cast. It makes it easier for me to understand what's going on. It's not a huge deal, but it is nice to confirm that nothing unexpected is happening with the cast.
briansmith
force-pushed
the
b/cast-1
branch
3 times, most recently
from
451083c to
b5c89a6
Compare
briansmith
force-pushed
the
b/cast-1
branch
4 times, most recently
from
7094852 to
7617036
Compare
|
I moved the slice aspect to #431. |
There was a problem hiding this comment.
Thanks for this!
I'll wait for @newpavlov to approve before merging because this does bump the MSRV, but I think that's fine.
| @@ -14,7 +14,7 @@ extern "C" { | |||
| } | |||
|
|
|||
| pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | |||
| let ret = unsafe { CCRandomGenerateBytes(dest.as_mut_ptr() as *mut c_void, dest.len()) }; | |||
| let ret = unsafe { CCRandomGenerateBytes(dest.as_mut_ptr().cast::<c_void>(), dest.len()) }; | |||
There was a problem hiding this comment.
I personally like having the type explicit in the cast. It makes it easier for me to understand what's going on. It's not a huge deal, but it is nice to confirm that nothing unexpected is happening with the cast.
|
I am fine with this change, but I think we can wait until v0.3 release, especially considering that it's just a code readability improvement. We also could use the new breaking release to relax our current MSRV policy. |
I'll merge this then, and I opened #433 to have discussion on if this and similar changes should count as a breaking change or not. |
Avoid anonymous type casts of the form
as *{const,mut} _.Make it clearer that we're never casting away constness by avoiding
as *mut Twhenever practical.