-
-
Notifications
You must be signed in to change notification settings - Fork 434
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
rand: Fix most clippy warnings #840
Conversation
This addresses parts of #838. |
The only failure is due to Miri being broken and thus unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for leaving this a while, I tend to ignore Clippy. There are a few pointless changes here and a few good ones.
@@ -233,7 +234,8 @@ where <R as BlockRngCore>::Results: AsRef<[u32]> + AsMut<[u32]> | |||
|
|||
#[inline(always)] | |||
fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> { | |||
Ok(self.fill_bytes(dest)) | |||
self.fill_bytes(dest); | |||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is to appease a lint intended to catch accidental semicolons, so arguably a false positive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I revert this? I think the new version is slightly more clear, but I don't have a strong preference.
The way I've handled Clippy suggestions previously is to accept the changes I think are useful and drop the rest. Is there a reason to do any more this time? |
They are provided by `core` since Rust 1.20.
Are you talking about the explicitly disabled lints? I think it is good to do that as well, as otherwise it is difficult to use clippy for Rand. I think we should use it more, because the |
I suppose we could make Clippy a little less pendantic by using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this clean-up @vks; overall it's a significant improvement.
One other thing I noticed though (probably we should add #[inline]
).
They are very small and usually just refering to the implementation of the underlying types.
Should be ready to merge now. |
Any idea why CI cannot clone |
I got confused with git's syntax and accidentally pushed to |
Aha, that's why I was confused about the clone command. I usually use the same branch name locally then can just do |
(That's what I usually do as well, but I had to make a fresh clone of Rand. To checkout out my remote branch, I had to use the |
@@ -188,6 +188,7 @@ where <R as BlockRngCore>::Results: AsRef<[u32]> + AsMut<[u32]> | |||
let read_u64 = |results: &[u32], index| { | |||
if cfg!(any(target_endian = "little")) { | |||
// requires little-endian CPU | |||
#[allow(clippy::cast_ptr_alignment)] // false positive | |||
let ptr: *const u64 = results[index..=index+1].as_ptr() as *const u64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
results
is aligned to 32bits, but then 64bits are read. I don't understand why this would be a false positive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess what we do with the result: ptr::read_unaligned(ptr)
No description provided.