Skip to content

treewide: tighten clippy lints #7483

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.

Sign up for GitHub

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

Jump to bottom
Merged
rbradford merged 6 commits into from
Nov 21, 2025
Merged

treewide: tighten clippy lints #7483

rbradford merged 6 commits into from
Nov 21, 2025

Conversation

@phip1611
Copy link
Member

@phip1611 phip1611 commented Nov 17, 2025 •
edited
Loading

Follow-up of #7482 to improve the code quality of Cloud Hypervisor in a sustainable way.
Most of the new individual lints are carefully selected from clippy::pedantic. The current PR reflects the number of lints Rob approved for the first lint tightening iteration.

Hints for Reviewers

  • I'd suggest we discuss and decide for each lint separately whether we want them. Therefore, I created one commit per lint for helping you finding a decision Rob already answered this
  • Please review commit by commit

Steps to Undraft

@phip1611 phip1611 requested a review from a team as a code owner November 17, 2025 15:21
@phip1611 phip1611 self-assigned this Nov 17, 2025
@phip1611 phip1611 marked this pull request as draft November 17, 2025 15:22
@phip1611 phip1611 changed the title add lint clippy::must_use_candidate to workspace [DRAFT, BLOCKED] add lint clippy::must_use_candidate to workspace Nov 17, 2025
@phip1611 phip1611 force-pushed the upstream-clippy2 branch 2 times, most recently from c506844 to a1371c9 Compare November 18, 2025 08:55
@phip1611 phip1611 changed the title [DRAFT, BLOCKED] add lint clippy::must_use_candidate to workspace treewide: tighten clippy lints Nov 18, 2025
@phip1611 phip1611 marked this pull request as ready for review November 18, 2025 08:56
@phip1611 phip1611 mentioned this pull request Nov 18, 2025
Closed
3 tasks
@rbradford
Copy link
Member

rbradford commented Nov 18, 2025

@phip1611 Everything up to "misc: clippy: add manual_string_new" seems very non controversial to me - can you split that into a separate PR that we can land. The rest I think we might need to think about some more.

@phip1611 phip1611 force-pushed the upstream-clippy2 branch 4 times, most recently from b215dfc to 8911af4 Compare November 20, 2025 08:50
rbradford
rbradford approved these changes Nov 20, 2025
View reviewed changes
Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

Thanks. lgtm to me - does anyone else have an opinion on this?

@phip1611 phip1611 force-pushed the upstream-clippy2 branch 3 times, most recently from 861cb59 to 9ea9ce4 Compare November 20, 2025 17:59
@phip1611
Copy link
Member Author

phip1611 commented Nov 20, 2025

I removed missing_const_for_fn for now from this PR as I'm unsure about the benefit in Cloud Hypervisor. It is very invasive. We should discuss this in a dedicated place.

@phip1611 phip1611 mentioned this pull request Nov 20, 2025
Closed
13 tasks
@phip1611 phip1611 force-pushed the upstream-clippy2 branch 2 times, most recently from 80abb66 to d78fae0 Compare November 20, 2025 18:48
likebreath
likebreath approved these changes Nov 20, 2025
View reviewed changes
Copy link
Member

@likebreath likebreath left a comment

Choose a reason for hiding this comment

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

Overall looks good. Not very sure about unnecessary_semicolon and semicolon_if_nothing_returned, but no strong opinions here.

@phip1611
Copy link
Member Author

phip1611 commented Nov 20, 2025 •
edited
Loading

Overall looks good. Not very sure about unnecessary_semicolon and semicolon_if_nothing_returned, but no strong opinions here.

Thanks! I don't have a strong opinion either. But if I read the lint descriptions, I have a tendency that it is beneficial.

@up2wing
Copy link
Contributor

up2wing commented Nov 21, 2025

LGTM.

I suppose this serial is good for the code quality, thx.

@phip1611
misc: clippy: add default clippy lint groups
d4fdda1 
This is the first commit in a series of commits to improve the Code
Quality in Cloud Hypervisor in a sustainable way. These are the
default rules from `clippy::all` but written here to be more explicit.
`clippy::all` refers to all "default sensible" lints, not all
existing lints.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: clippy: add unnecessary_semicolon
5df3d0e 
Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: clippy: add uninlined_format_args
497ba7b 
Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: clippy: add semicolon_if_nothing_returned
25e9935 
Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: clippy: add manual_string_new
4002841 
Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
vmm: unrelated small code improvements
edfc787 
Unfortunately, there is no lint for that.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@rbradford rbradford enabled auto-merge November 21, 2025 08:46
@phip1611
Copy link
Member Author

phip1611 commented Nov 21, 2025

I don't quite see how this suddenly fails but never failed before? https://github.com/cloud-hypervisor/cloud-hypervisor/actions/runs/19564834479/job/56024328565?pr=7483

@rbradford rbradford added this pull request to the merge queue Nov 21, 2025
Merged via the queue into cloud-hypervisor:main with commit f02745a Nov 21, 2025
43 of 45 checks passed
@phip1611 phip1611 mentioned this pull request Nov 21, 2025
Merged
@phip1611 phip1611 deleted the upstream-clippy2 branch November 21, 2025 15:20
@phip1611 phip1611 mentioned this pull request Nov 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rbradford rbradford rbradford approved these changes

@up2wing up2wing up2wing approved these changes

@alyssais alyssais alyssais approved these changes

@likebreath likebreath likebreath approved these changes

Assignees

@phip1611 phip1611

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@phip1611 @rbradford @up2wing @alyssais @likebreath