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

restrict manual_clamp to const case, bring it out of nursery #12543

Merged
merged 4 commits into from Mar 29, 2024

Conversation

Xaeroxe
Copy link
Contributor

@Xaeroxe Xaeroxe commented Mar 24, 2024

Implements the plan that I described in #9484 (comment)

This does two things primarily

  1. Restrict manual_clamp such that it will only trigger if we are able to guarantee that clamp won't panic at runtime.
  2. Bring manual_clamp out of nursery status and move it into the complexity group.

changelog: [manual_clamp]: Restrict this lint such that it only triggers if max and min are const, and max is greater than or equal to min. Then bring it out of the nursery group.

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2024

r? @xFrednet

rustbot has assigned @xFrednet.
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 Mar 24, 2024
@xFrednet
Copy link
Member

Hey @GuillaumeGomez, do you want to take a look at this PR?

@GuillaumeGomez
Copy link
Member

Sure!

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Apart from my nit, lint group and code changes both look good to me.

clippy_lints/src/manual_clamp.rs Outdated Show resolved Hide resolved
Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks!

Handing back review to @xFrednet now. ;)

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Two small NITs and then this should be good to go. Thank you for coming back and fixing the lint!

Also thank you to @GuillaumeGomez for the review :)

clippy_lints/src/manual_clamp.rs Show resolved Hide resolved
clippy_lints/src/manual_clamp.rs Outdated Show resolved Hide resolved
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Mar 29, 2024

@xFrednet resolved in 89588f4

@xFrednet
Copy link
Member

Looks good to me, thank you! :D

@bors r=xFrednet,GuillaumeGomez

@bors
Copy link
Collaborator

bors commented Mar 29, 2024

📌 Commit 89588f4 has been approved by xFrednet,GuillaumeGomez

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 29, 2024

⌛ Testing commit 89588f4 with merge 971e435...

@bors
Copy link
Collaborator

bors commented Mar 29, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet,GuillaumeGomez
Pushing 971e435 to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented Mar 29, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet,GuillaumeGomez
Pushing 971e435 to master...

@bors bors merged commit 971e435 into rust-lang:master Mar 29, 2024
5 checks passed
@bors
Copy link
Collaborator

bors commented Mar 29, 2024

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

@Xaeroxe Xaeroxe deleted the manual-clamp-const branch March 29, 2024 19:50
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.

None yet

5 participants