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

cast_possible_truncation: Fix some false-positive cases #12962

Closed
wants to merge 8 commits into from

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jun 19, 2024

Fix partially #7486 (comment).
Fix #9613.
changelog: [cast_possible_truncation]: fix some false-positive on % operator, valid constants, and size_of

@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2024

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 19, 2024
@tesuji tesuji changed the title Cast truncating cast_possible_truncation: Fix some false-positve Jun 19, 2024
@tesuji tesuji changed the title cast_possible_truncation: Fix some false-positve cast_possible_truncation: Fix some false-positive cases Jun 19, 2024
@tesuji tesuji force-pushed the cast-truncating branch 3 times, most recently from defa4af to 8fea999 Compare June 24, 2024 13:42
@tesuji tesuji force-pushed the cast-truncating branch 3 times, most recently from 0f2daf3 to e86a94c Compare July 2, 2024 14:03
Require all fields to be local
@xFrednet
Copy link
Member

xFrednet commented Aug 3, 2024

Hey, this is a ping from triage. @Jarcho can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@rustbot ready

@Jarcho
Copy link
Contributor

Jarcho commented Aug 4, 2024

Looks ok. Thank you @bors r+

@bors
Copy link
Collaborator

bors commented Aug 4, 2024

📌 Commit 3770c8f has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 4, 2024

⌛ Testing commit 3770c8f with merge 436f9ee...

bors added a commit that referenced this pull request Aug 4, 2024
cast_possible_truncation: Fix some false-positive cases

Fix partially  #7486 (comment).
Fix #9613.
changelog: [`cast_possible_truncation`]: fix some false-positive on % operator, valid constants, and size_of
@bors
Copy link
Collaborator

bors commented Aug 4, 2024

💔 Test failed - checks-action_test

@Jarcho
Copy link
Contributor

Jarcho commented Aug 6, 2024

@tesuji you may need to rebase to fix the errors.

@tesuji
Copy link
Contributor Author

tesuji commented Aug 7, 2024

I think I'm stuck. I don't know any ways to bless test for 32-bit target. Running with cargo uibless --target i686-unknown-linux-gnu fail with error says that compiletest doesn't recognize --target option. Could you me give any pointers to workaround this?

@Jarcho
Copy link
Contributor

Jarcho commented Aug 7, 2024

The uibless command is defined in .cargo/config.toml as test --test compile-test -- -- --bless. You can edit that as needed.

@tesuji
Copy link
Contributor Author

tesuji commented Aug 7, 2024

It's complicated than I expected:

> rustup install nightly-2024-07-25-i686-unknown-linux-gnu -c rust-src rustc-dev llvm-tools-preview
> # also install i386 zlib
> cargo +i686-unknown-linux-gnu uitest

But hey, since I added this condition to_nbits < from_nbits && to_nbits <= 32, on line 137, the diff will be different
between 32-bti and 64-bit targets. How do I mark this with compiletest?

@Jarcho
Copy link
Contributor

Jarcho commented Aug 7, 2024

That condition should be to_nbits <= from_nbits || to_nbits <= 32. We always want to lint usize to u32 casts as they're not portable.

@tesuji
Copy link
Contributor Author

tesuji commented Aug 7, 2024

If the constant is locally defined and have value < u32::MAX, it will be linted again when changes the condition.

I don't have time or enough motivation for developing clippy in general. Closing this for now.

Anyway, thank you very much for your reviewing, Jarcho.

@tesuji tesuji closed this Aug 7, 2024
@tesuji tesuji deleted the cast-truncating branch August 7, 2024 03:43
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.

cast_possible_truncation false positive: const defined in the same crate
5 participants