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

Style: do not defensively use saturating_sub() #13513

Merged
merged 1 commit into from
Oct 6, 2024

Conversation

samueltardieu
Copy link
Contributor

Using saturating_sub() here in code which cannot fail brings a false sense of security. If for any reason a logic error was introduced and caused self.loop_depth to reach 0 before being decremented, using saturating_sub(1) would silently mask the programming error instead of panicking loudly as it should (at least in dev profile).

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2024

r? @Manishearth

rustbot has assigned @Manishearth.
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 6, 2024
@y21
Copy link
Member

y21 commented Oct 6, 2024

Sgtm 👍

@bors r+

@bors
Copy link
Contributor

bors commented Oct 6, 2024

📌 Commit 7018975 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 6, 2024

⌛ Testing commit 7018975 with merge bc9a183...

bors added a commit that referenced this pull request Oct 6, 2024
Style: do not defensively use `saturating_sub()`

Using `saturating_sub()` here in code which cannot fail brings a false sense of security. If for any reason a logic error was introduced and caused `self.loop_depth` to reach 0 before being decremented, using `saturating_sub(1)` would silently mask the programming error instead of panicking loudly as it should (at least in dev profile).

changelog: none
Using `saturating_sub()` here in code which cannot fail brings a false
sense of security. If for any reason a logic error was introduced and
caused `self.loop_depth` to reach 0 before being decremented, using
`saturating_sub(1)` would silently mask the programming error instead of
panicking loudly as it should (at least in dev profile).
@samueltardieu
Copy link
Contributor Author

Fun fact, I force-pushed (with only a new commit message, content unchanged) after you r+'d it. I guess bors will keep the previous one, which is fine.

@y21
Copy link
Member

y21 commented Oct 6, 2024

The force push might have confused bors 🤔
Let's try again, @bors r+

@bors
Copy link
Contributor

bors commented Oct 6, 2024

📌 Commit af6816c has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 6, 2024

⌛ Testing commit af6816c with merge 1c9e5d0...

@bors
Copy link
Contributor

bors commented Oct 6, 2024

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

@bors bors merged commit 1c9e5d0 into rust-lang:master Oct 6, 2024
8 checks passed
@samueltardieu samueltardieu deleted the push-zvvzytrovulq branch October 6, 2024 17:24
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.

5 participants