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.

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

Add clippy::cast_lossless #3216

Merged
merged 2 commits into from
Nov 21, 2022
Merged

Add clippy::cast_lossless #3216

merged 2 commits into from
Nov 21, 2022

Conversation

StemCll
Copy link
Contributor

@StemCll StemCll commented Oct 27, 2022 •
edited
Loading

Changes

Adds clippy::cast_lossless warning.

Reason

Closes #3196

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

  • All commits in this PR are signed (git commit -s).
  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • New unsafe code is documented.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • This functionality can be added in rust-vmm.

@StemCll StemCll marked this pull request as ready for review October 27, 2022 20:05
@StemCll
Copy link
Contributor Author

StemCll commented Oct 31, 2022

Hey @JonathanWoollett-Light I've fixed warnings for aarch64 CI

@StemCll StemCll force-pushed the clippy_cast_lossless branch from 4737748 to c790a5b Compare October 31, 2022 17:12
JonathanWoollett-Light
JonathanWoollett-Light suggested changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@JonathanWoollett-Light JonathanWoollett-Light left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great work!

On ARM you will need to update the code coverage values. Pick the outputs given in the failing tests (82.41 for 5.10 and 79.60 for 4.14) then paste these value as the new targets under tests/integration_tests/build/test_coverage.py.

I see some changes to tests/integration_tests/build/test_clippy.py are included, it would best to avoid changes to this in this PR.

@StemCll StemCll force-pushed the clippy_cast_lossless branch from c790a5b to f709c7c Compare November 2, 2022 14:51
@StemCll
Copy link
Contributor Author

StemCll commented Nov 2, 2022 •
edited
Loading

I've pushed the updated values in test_coverage.py.

Regarding the test_clippy.py - as far as I can see, the black is executed in pre-commit script. What should I do in this case? Add this file without executing black? 😄

@JonathanWoollett-Light
Copy link
Contributor

the black is executed in pre-commit script. What should I do in this case? Add this file without executing black? 😄

Yep, using the --no-verify git commit argument should skip it.

It'd be worth opening a separate PR for the black reformat here.

@StemCll StemCll mentioned this pull request Nov 2, 2022
Merged
9 tasks
@StemCll StemCll force-pushed the clippy_cast_lossless branch from 992ffb9 to e7c966f Compare November 2, 2022 15:17
@StemCll
Copy link
Contributor Author

StemCll commented Nov 2, 2022

Separate PR for black reformat is ready here - #3232

I've reverted the formatting here.

Also - should this clippy lint be added to bindgen? Same as clippy::ptr_as_ptr?

@JonathanWoollett-Light
Copy link
Contributor

JonathanWoollett-Light commented Nov 2, 2022 •
edited
Loading

Also - should this clippy lint be added to bindgen? Same as clippy::ptr_as_ptr?

It doesn't matter too much if it doesn't trigger any warnings, but I'd lean towards yes to avoid future errors.

@StemCll
Copy link
Contributor Author

StemCll commented Nov 2, 2022

Done ✔️

@JonathanWoollett-Light
Copy link
Contributor

JonathanWoollett-Light commented Nov 2, 2022 •
edited
Loading

Please squash all your changes into 1 commit for implementing the lint and 1 commit for the code coverage changes.

@StemCll StemCll force-pushed the clippy_cast_lossless branch 2 times, most recently from cfa300b to 0aec9ce Compare November 3, 2022 16:43
@StemCll
Copy link
Contributor Author

StemCll commented Nov 3, 2022

done ✔️

dianpopa
dianpopa previously requested changes Nov 7, 2022
View reviewed changes
src/api_server/src/lib.rs Show resolved Hide resolved
@StemCll StemCll force-pushed the clippy_cast_lossless branch from cc53056 to fa55c88 Compare November 7, 2022 14:20
@StemCll StemCll force-pushed the clippy_cast_lossless branch from fa55c88 to 6c453d3 Compare November 16, 2022 15:50
@JonathanWoollett-Light
Copy link
Contributor

JonathanWoollett-Light commented Nov 16, 2022 •
edited
Loading

You can rename the 2nd commit to simply Update code coverage for Arm.

@StemCll StemCll force-pushed the clippy_cast_lossless branch from 6c453d3 to be20a67 Compare November 16, 2022 17:13
@StemCll
Copy link
Contributor Author

StemCll commented Nov 16, 2022

@JonathanWoollett-Light done ✔️

@JonathanWoollett-Light
Copy link
Contributor

Getting failures for Arm coverage being higher, the 2nd commit needs to be adjusted to match (79.66 and 82.47 need to be set on Arm for 4.14 and 5.10 respectively).

@StemCll StemCll force-pushed the clippy_cast_lossless branch from be20a67 to 05127c5 Compare November 16, 2022 18:25
dianpopa
dianpopa reviewed Nov 17, 2022
View reviewed changes
src/io_uring/src/lib.rs Outdated Show resolved Hide resolved
@dianpopa dianpopa dismissed their stale review November 17, 2022 13:15

Decided on way forward

@StemCll StemCll force-pushed the clippy_cast_lossless branch from 42cdc44 to 178c768 Compare November 20, 2022 10:56
@StemCll StemCll force-pushed the clippy_cast_lossless branch from 178c768 to ed8c793 Compare November 20, 2022 11:04
@dianpopa dianpopa merged commit 390a007 into firecracker-microvm:main Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonathanWoollett-Light JonathanWoollett-Light JonathanWoollett-Light approved these changes

@dianpopa dianpopa dianpopa approved these changes

Assignees

@StemCll StemCll

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use clippy::cast_lossless
3 participants
@StemCll @JonathanWoollett-Light @dianpopa