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

Add manual_bits lint #8213

Merged
merged 1 commit into from
Jan 12, 2022
Merged

Add manual_bits lint #8213

merged 1 commit into from
Jan 12, 2022

Conversation

paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Jan 3, 2022

Closes #6670


changelog: new lint: [manual_bits]

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 3, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 3, 2022

Oh nice! Thanks for implementing this! (I'm not a clippy reviewer. Just happy to see this getting added. ^^)

@ghost
Copy link

ghost commented Jan 4, 2022

It looks like it triggers for size_of_val as well. I don't think it should do this.

#![warn(clippy::size_of_as_bits)]

use std::mem;

fn main() {
    let x = 0u32;
    println!("{}", 8 * mem::size_of_val(&x));
}
warning: you seem to be using `mem::size_of::<T>()` to obtain the size of `T` in bits
 --> src/main.rs:7:20
  |
7 |     println!("{}", 8 * mem::size_of_val(&x));
  |                    ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `u32::BITS`
  |
note: the lint level is defined here
 --> src/main.rs:1:9
  |
1 | #![warn(clippy::size_of_as_bits)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#size_of_as_bits

@ghost
Copy link

ghost commented Jan 4, 2022

There is also an issue with type aliases.

#![warn(clippy::size_of_as_bits)]

use std::mem;

type Word = u32;

fn main() {
    println!("{}", 8 * mem::size_of::<Word>());
}
warning: you seem to be using `mem::size_of::<T>()` to obtain the size of `T` in bits
 --> src/main.rs:8:20
  |
8 |     println!("{}", 8 * mem::size_of::<Word>());
  |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `u32::BITS`
  |
note: the lint level is defined here
 --> src/main.rs:1:9
  |
1 | #![warn(clippy::size_of_as_bits)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#size_of_as_bits

I think the suggestion should be Word::BITS instead.

@camsteffen
Copy link
Contributor

The name is a bit funny to me. Perhaps size_of_to_bits or bits_from_size_of.

@ghost
Copy link

ghost commented Jan 5, 2022

I like manual_bits for the name.

@paolobarbolini paolobarbolini changed the title Add size_of_as_bits lint Add manual_bits lint Jan 8, 2022
@paolobarbolini
Copy link
Contributor Author

I applied all of the above suggestions

@bors
Copy link
Collaborator

bors commented Jan 9, 2022

☔ The latest upstream changes (presumably #8236) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Collaborator

bors commented Jan 11, 2022

☔ The latest upstream changes (presumably #8210) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

I also like the manual_bits name. This is similar to the manual_memcpy name (and lint).

Small comment for the error message. This will require a cargo dev bless. Otherwise LGTM.

clippy_lints/src/manual_bits.rs Outdated Show resolved Hide resolved
@paolobarbolini
Copy link
Contributor Author

👍. I also rebased on top of master since #8190 would have broken my code and bumped the clippy::version to 1.60.

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Jan 12, 2022

📌 Commit 166737f has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Jan 12, 2022

⌛ Testing commit 166737f with merge 7c82ae1...

@bors
Copy link
Collaborator

bors commented Jan 12, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 7c82ae1 to master...

@bors bors merged commit 7c82ae1 into rust-lang:master Jan 12, 2022
@HaoYang670
Copy link

HaoYang670 commented Apr 10, 2022

I am afraid that in most contexts (especially in match arms), sizeof::<u8>() * 8 cannot be simply replaced by u8::BITS because the return type of sizeof is usize but u8::BITS is in u32 type.

What about using "help: consider using: u32::BITS as usize "?

@ghost
Copy link

ghost commented Apr 10, 2022

@HaoYang670, what is the point of tacking a comment onto a PR that was merged three months ago? The correct way to get your problem addressed is to create an issue.

@xFrednet
Copy link
Member

xFrednet commented Apr 10, 2022

I'll fix the suggestion, that'll probably be faster than creating an issue for it ^^

bors added a commit that referenced this pull request Apr 14, 2022
Add `usize` cast to `clippy::manual_bits` suggestion

A fix for the suggestion from #8213

changelog: [`manual_bits`]: The suggestion now includes a cast for proper type conversion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace std::mem::size_of<T>() with ::BITS constant
8 participants