Skip to content

Lint casts to u128 in cast_lossless #13146

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

Merged
merged 1 commit into from
Jul 24, 2024
Merged

Conversation

Alexendoo
Copy link
Member

@Alexendoo Alexendoo commented Jul 22, 2024

Reverts #12496 per https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Should.20.60as.20u128.60.20trigger.20cast_lossless

Also changes the lint messages and refactors the suggestion production - Fixes #12695

changelog: [cast_lossless]: lint casts to u128

@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2024

r? @y21

rustbot has assigned @y21.
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 Jul 22, 2024
@Alexendoo Alexendoo force-pushed the cast-lossless-128 branch from 877ac4c to 610c5e2 Compare July 22, 2024 20:00
@xFrednet
Copy link
Member

I think the linked zulip thread is wrong ^^

@Alexendoo could you link the correct one?

@Alexendoo Alexendoo force-pushed the cast-lossless-128 branch from 610c5e2 to 8fa2d85 Compare July 23, 2024 15:05
match cast_to_hir.kind {
TyKind::Infer => {
let sugg = if cast_from_expr.precedence().order() == PREC_PREFIX {
format!("({from}")
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's a missing ) here

Copy link
Member

Choose a reason for hiding this comment

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

Also, can this just use Sugg::hir().maybe_par() or does that not work here? I believe the current version won't handle parenthesized expressions like (1i8 + 1i8) as _ correctly. Would need < PREC_POSTFIX or something like that I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

🙃 I wonder why I did not add tests for this

It was meant to be from_snippet rather than from so that PREC_PREFIX would cover the unary op cases, but that still missed x as u16 as _

I didn't use it initially because Sugg had an issue with macro spans but fixing it was easier than I thought so it does now

@y21
Copy link
Member

y21 commented Jul 23, 2024

Also suggesting .into() for as _ is a nice idea, much better than the <_>::from() I had in mind

@Alexendoo Alexendoo force-pushed the cast-lossless-128 branch from 8fa2d85 to 6d28e1a Compare July 24, 2024 14:33
@y21
Copy link
Member

y21 commented Jul 24, 2024

Nice, looks all good to me now, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 24, 2024

📌 Commit 6d28e1a has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 24, 2024

⌛ Testing commit 6d28e1a with merge 5e6540f...

@bors
Copy link
Contributor

bors commented Jul 24, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 5e6540f to master...

@bors bors merged commit 5e6540f into rust-lang:master Jul 24, 2024
8 checks passed
@Alexendoo Alexendoo deleted the cast-lossless-128 branch July 24, 2024 21:44
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-lossless suggests calling macro in itself
5 participants