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

Fixed some clippy warnings in components #32025

Merged
merged 2 commits into from
Apr 10, 2024
Merged

Conversation

jahielkomu
Copy link
Contributor

@jahielkomu jahielkomu commented Apr 9, 2024

This PR fixes some clippy warnings by;

  • Removing unused lifetimes
  • Simplifying boolean expressions
  • Removing useless use of format!
  • Replacing the use Option.and_then() with .map()
  • Replacing the call of Option.map().flatten() with .and_then()

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes are part of Fix as many clippy problems as possible #31500
  • These changes do not require tests because it doesn't touch functionality.

@jahielkomu
Copy link
Contributor Author

Hello @mrobinson
Please kindly review.
Thanks.

@jahielkomu
Copy link
Contributor Author

jahielkomu commented Apr 9, 2024

@mrobinson , a lot of the warnings talk about the use of unsafe code, should we use #[allow(unsafe_code)] ?
Kindly advise.

Comment on lines 1192 to 1193
let remove_from_node =
!(!move_start || !move_end && node_is_end) || move_end && !node_is_start;
Copy link
Member

Choose a reason for hiding this comment

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

Hrm...I think this new formulation is more difficult to understand. In this case, I would add an exception for the clippy warning or rewrite the logic to be more understandable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I absolutely agree with you @mrobinson ...I picked this straight from the clippy suggestion and felt it was more complicated but thought it was the standard. I checked that it evaluated to the same as the before and applied it.
Let me find another way for this sir.
Thank you.

Comment on lines 1254 to 1255
let remove_from_node =
!(!move_start || !move_end && node_is_end) || move_end && !node_is_start;
Copy link
Member

Choose a reason for hiding this comment

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

I feel the same way about this new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me work on it sir

@jahielkomu
Copy link
Contributor Author

I have updated the PR @mrobinson , kindly review again.

@mrobinson mrobinson added this pull request to the merge queue Apr 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 9, 2024
@jahielkomu
Copy link
Contributor Author

jahielkomu commented Apr 9, 2024

Hmm something seems to not be going fine sir @mrobinson .
Is it due to the changes made in the PR?
./mach test-unit, ./mach test-tidy and ./mach build -d ran with no errors.
Let me rerun them perhaps

->They are running with no errors.

@jahielkomu
Copy link
Contributor Author

@mrobinson , a lot of the warnings talk about the use of unsafe code, should we use #[allow(unsafe_code)] ? Kindly advise.

Sir would you recommend this?

@mrobinson mrobinson added this pull request to the merge queue Apr 10, 2024
@mrobinson
Copy link
Member

@mrobinson , a lot of the warnings talk about the use of unsafe code, should we use #[allow(unsafe_code)] ? Kindly advise.

Adding allow(unsafe_code) shouldn't be necessary, as it's absence would cause a compilation error. I think it would be helpful to see the actual error messages that you are referring to in the clippy warnings. If they are the ones regarding the missing "# Safety" section in the documentation, the fix is to write that documentation. That requires a bit of understanding and care though, as we want the documentation to actually reflect the real safety requirements of those functions.

Merged via the queue into servo:main with commit 89a4820 Apr 10, 2024
9 checks passed
@jahielkomu jahielkomu deleted the clippy/fix branch April 18, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants