Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace more casts with safer conversions & enable cast-related lints. #445

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

briansmith
Copy link
Contributor

No description provided.

@briansmith briansmith changed the title B/casts 3 2 Replace more casts with safer conversions & enable cast-related lints. May 31, 2024
@briansmith briansmith force-pushed the b/casts-3-2 branch 3 times, most recently from d29aa27 to 125a3e6 Compare May 31, 2024 23:40
src/util_libc.rs Outdated Show resolved Hide resolved
@briansmith briansmith marked this pull request as ready for review June 1, 2024 00:12
@briansmith briansmith force-pushed the b/casts-3-2 branch 2 times, most recently from d9b4a3a to 90b24d4 Compare June 1, 2024 18:59
@briansmith
Copy link
Contributor Author

I have rebased this on top of #447 and extended it to address additional Clippy feedback from that change.

@briansmith briansmith force-pushed the b/casts-3-2 branch 13 times, most recently from f331ab0 to 67f3bd5 Compare June 2, 2024 20:25
@briansmith
Copy link
Contributor Author

I greatly cleaned this up and moved non-type-conversion-related fixes to separate PRs. For example, the Solaris changes are now in #448.

I also separated this from #447 since there is an open discussion about how to expand the Clippy usage.

@briansmith briansmith force-pushed the b/casts-3-2 branch 2 times, most recently from 4008b07 to 7a89e48 Compare June 2, 2024 20:47
@briansmith briansmith marked this pull request as draft June 6, 2024 19:29
@briansmith
Copy link
Contributor Author

Switching this to be a draft PR because PRs #458, #459, and #461 ended up fixing most of these. I will rebase this after those PRs have been reviewed.

Comment on lines +211 to +225
#![deny(
clippy::cast_lossless,
clippy::cast_possible_truncation,
clippy::cast_possible_wrap,
clippy::cast_precision_loss,
clippy::cast_ptr_alignment,
clippy::cast_sign_loss,
clippy::char_lit_as_u8,
clippy::checked_conversions,
clippy::fn_to_numeric_cast,
clippy::fn_to_numeric_cast_with_truncation,
clippy::ptr_as_ptr,
clippy::unnecessary_cast,
clippy::useless_conversion
)]
Copy link
Member

@josephlr josephlr Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to deny entire categories of lints (pedantic, correctness, complexity, etc...) rather than specific ones?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, with the number of additional lints it may be better to enable them in clippy.toml.

@briansmith briansmith force-pushed the b/casts-3-2 branch 2 times, most recently from d06067f to 4f8d78d Compare June 21, 2024 03:38
@@ -27,7 +27,9 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
// Prevent overflow of u32
let chunk_size = usize::try_from(i32::MAX).expect("Windows does not support 16-bit targets");
for chunk in dest.chunks_mut(chunk_size) {
let ret = unsafe { RtlGenRandom(chunk.as_mut_ptr().cast::<c_void>(), chunk.len() as u32) };
#[allow(clippy::cast_possible_truncation)]
let chunk_len = chunk.len() as u32;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can write it as chunk.len().try_into().expect("chunk size is bounded by i32::MAX"). Compiler is able to properly remove such panic.

// The chunk can be smaller than buf's length, so we call to
// JS to create a smaller view of buf without allocation.
#[allow(clippy::cast_possible_truncation)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use .try_into().expect("..") here same as in windows7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants