Skip to content

Conversation

@blyxyas
Copy link
Member

@blyxyas blyxyas commented Oct 10, 2024

Fixes #13524

changelog: [implicit_saturating_sub]: Fix span issue on else blocks

@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2024

r? @dswij

rustbot has assigned @dswij.
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 Oct 10, 2024
Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@dswij
Copy link
Member

dswij commented Oct 11, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2024

📌 Commit 140a127 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 11, 2024

⌛ Testing commit 140a127 with merge 14c05c9...

@bors
Copy link
Contributor

bors commented Oct 11, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 14c05c9 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Oct 11, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 14c05c9 to master...

@bors bors merged commit 14c05c9 into rust-lang:master Oct 11, 2024
8 checks passed
@bors
Copy link
Contributor

bors commented Oct 11, 2024

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

Comment on lines +230 to +234
} else if a >= b {
b.saturating_sub(a)
} else {
b - a
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit late, but reading through #13524, shouldn't the fixed code be just } else { b.saturating_sub(a) } (old suggestion was almost correct but it was missing braces)?

The new suggestion adds more code instead of simplifying it like before (changes a 0 to saturating_sub instead of the whole if)? 👀

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 did some more debugging, and yep, it was a mistake, #13542 is now open reverting this and implementing a more robust solution ❤️

bors added a commit that referenced this pull request Oct 13, 2024
[`implicit_saturating_sub`] Fix suggestion with a less volatile approach

Related to #13533, such and obvious mistake got pass my watch, quite embarassing :/

Revert #13533 and implement a more robust solution.

Revert "Fix span issue on `implicit_saturating_sub`
This reverts commit 140a127.

changelog: [`lint_name`]: Fix suggestion for `if {} else if {} else {}` cases

r? `@y21`
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.

Compiler error when using --fix on clippy::implicit_saturating_sub

5 participants