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

rand: Fix most clippy warnings #840

Merged
merged 28 commits into from Jul 25, 2019
Merged

rand: Fix most clippy warnings #840

merged 28 commits into from Jul 25, 2019

Conversation

vks
Copy link
Collaborator

@vks vks commented Jul 9, 2019

No description provided.

@vks
Copy link
Collaborator Author

vks commented Jul 9, 2019

This addresses parts of #838.

@vks
Copy link
Collaborator Author

vks commented Jul 9, 2019

The only failure is due to Miri being broken and thus unrelated.

Copy link
Member

@dhardy dhardy left a 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.

rand_core/src/block.rs Outdated Show resolved Hide resolved
@@ -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(())
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

rand_distr/src/dirichlet.rs Show resolved Hide resolved
src/distributions/utils.rs Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Jul 22, 2019

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.
@vks
Copy link
Collaborator Author

vks commented Jul 23, 2019

Is there a reason to do any more this time?

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 cast_ptr_alignment lint would have found some of the undefined behavior exposed by Miri.

@dhardy
Copy link
Member

dhardy commented Jul 23, 2019

I suppose we could make Clippy a little less pendantic by using #![allow(clippy::style)]. Would that make it easier to see the important suggestions?

Copy link
Member

@dhardy dhardy left a 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]).

src/seq/index.rs Show resolved Hide resolved
They are very small and usually just refering to the implementation of the
underlying types.
@vks
Copy link
Collaborator Author

vks commented Jul 25, 2019

Should be ready to merge now.

@dhardy
Copy link
Member

dhardy commented Jul 25, 2019

Any idea why CI cannot clone vks/clippy?

@vks
Copy link
Collaborator Author

vks commented Jul 25, 2019

I got confused with git's syntax and accidentally pushed to rust-random/rand (branch vks/rand) instead of vks/rand (branch clippy). I since then deleted the bogus vks/clippy branch, which is why Travis complains. However, the Travis build for the correct clippy branch succeeded.

@dhardy
Copy link
Member

dhardy commented Jul 25, 2019

Aha, that's why I was confused about the clone command. I usually use the same branch name locally then can just do cargo push dhardy.

@dhardy dhardy merged commit 684aa8f into rust-random:master Jul 25, 2019
@vks vks deleted the clippy branch July 25, 2019 15:45
@vks
Copy link
Collaborator Author

vks commented Jul 25, 2019

Aha, that's why I was confused about the clone command. I usually use the same branch name locally then can just do cargo push dhardy.

(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 vks/clippy name. However, for some reason git was not tracking vks/rand, so git push pushed to rust-random/rand instead.)

@vks vks mentioned this pull request Jul 25, 2019
3 tasks
@@ -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;
Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

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

rust-lang/rust-clippy#2881

Guess what we do with the result: ptr::read_unaligned(ptr)

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