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

clippy: Fix warnings in generated code #31721

Merged
merged 4 commits into from Mar 17, 2024
Merged

clippy: Fix warnings in generated code #31721

merged 4 commits into from Mar 17, 2024

Conversation

eerii
Copy link
Contributor

@eerii eerii commented Mar 17, 2024

Fixes some clippy warnings from the code generated by CodegenRust.py. It specifically gets rids of all errors from these files.

Due to the nature of the generated code, some allowances had to be made, sometimes because of false positives, and sometimes because "fixing" the warning would be very inefficient or require changing how everything generates. The allows and their reasoning are the following:

  • approx_constant: Values defined in .webidl files could be related to constants such as pi. In rust it would be better to use defined types such as f64::consts::PI, but because these are taken from other files they should stay as numeric values.
  • let_unit_value: In CGCallGenerator the value of result can take the () type. This is intended, and while specific allows could be placed for every let binding easily, this generates too many lines so it may be better to allow it for the entire file. There is also discussion regarding this lint in let_unit_value should be allowed when I explicitly write "let () = ..." rust-lang/rust-clippy#9048.
  • needless_return: In CGPerSignatureCall, the call to wrap_return_value can be straightforward, so a simple true at the end could suffice, but it could also contain nested matches, so return true; becomes necessary. Distinguishing between all the possible cases seems overly complicated when return true; works and isn't much more verbose. The other instances of returning were addressed.
  • too_many_arguments: Like in other cases, some functions generate with too many arguments. It seems counterproductive to check every function to know if it has more than x arguments and add an allow for it automatically when it can be permitted for every file.
  • unnecessary_cast: This is specifically for MethodDefiner::generateArray. There is a comment specifying that it's not easy to tell whether the methodinfo is a JSJitInfo or a JSTypedMethodJitInfo, so we cast them both to JSJitInfo and rely on the compiler doing the conversion. This creates many unnecessary_cast warnings for JSJitInfo, but it is documented and not trivial to fix.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes are a part of Fix as many clippy problems as possible #31500
  • These changes do not require tests because they do not change functionality, only fix warnings

Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

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

Nice simplifications!

@jdm jdm added this pull request to the merge queue Mar 17, 2024
Merged via the queue into servo:main with commit 99ddab4 Mar 17, 2024
10 checks passed
@eerii eerii deleted the clippy_generated branch March 18, 2024 10:38
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