Skip to content

Conversation

samueltardieu
Copy link
Member

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
Member 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