Skip to content

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

Merged
bors merged 4 commits into
rust-lang:masterfrom
Xaeroxe:manual-clamp-const
Mar 29, 2024
Merged

restrict manual_clamp to const case, bring it out of nursery#12543
bors merged 4 commits into
rust-lang:masterfrom
Xaeroxe:manual-clamp-const

Conversation

@Xaeroxe

@Xaeroxe Xaeroxe commented Mar 24, 2024

Copy link
Copy Markdown
Contributor

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

rustbot commented Mar 24, 2024

Copy link
Copy Markdown
Collaborator

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
@Xaeroxe
Xaeroxe force-pushed the manual-clamp-const branch from 4484597 to 0cf9d9c Compare March 24, 2024 03:29
@xFrednet

Copy link
Copy Markdown
Contributor

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

@GuillaumeGomez

Copy link
Copy Markdown
Member

Sure!

@GuillaumeGomez GuillaumeGomez left a comment

Copy link
Copy Markdown
Member

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.

Comment thread clippy_lints/src/manual_clamp.rs Outdated
Comment thread clippy_lints/src/manual_clamp.rs Outdated

@GuillaumeGomez GuillaumeGomez left a comment

Copy link
Copy Markdown
Member

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. ;)

@xFrednet xFrednet left a comment

Copy link
Copy Markdown
Contributor

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 :)

Comment thread clippy_lints/src/manual_clamp.rs
Comment thread clippy_lints/src/manual_clamp.rs Outdated
@Xaeroxe

Xaeroxe commented Mar 29, 2024

Copy link
Copy Markdown
Contributor Author

@xFrednet resolved in 89588f4

@xFrednet

Copy link
Copy Markdown
Contributor

Looks good to me, thank you! :D

@bors r=xFrednet,GuillaumeGomez

@bors

bors commented Mar 29, 2024

Copy link
Copy Markdown
Contributor

📌 Commit 89588f4 has been approved by xFrednet,GuillaumeGomez

It is now in the queue for this repository.

@bors

bors commented Mar 29, 2024

Copy link
Copy Markdown
Contributor

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

@bors

bors commented Mar 29, 2024

Copy link
Copy Markdown
Contributor

☀️ 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

bors commented Mar 29, 2024

Copy link
Copy Markdown
Contributor

☀️ 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
@bors

bors commented Mar 29, 2024

Copy link
Copy Markdown
Contributor

👀 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.

5 participants