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 remaining warnings in generated code #31844

Merged
merged 2 commits into from Mar 25, 2024

Conversation

eerii
Copy link
Contributor

@eerii eerii commented Mar 23, 2024

Followup to #31721. Fixes all the remaining warnings generated by CodegenRust.py (at least on linux).

Notable changes:

  • When using CGIfElseWrapper, nested if {} else { if {} else {} } statements are now converted to if {} else if {} else {}.
  • wrap_panic invocations that return a result now check if the body contains an explicit return statement. If it doesn't, it can get rid of an extra closure that was giving warnings.
  • Changes the Default implementation of EventInit and ExtendableEventInit to use the already defined empty methods they have.
  • Adds allows for upper case acronyms and repeated enum variant names to the relevant modules, since most of the names come from the spec.
  • Renames the warning list I added last time from IGNORED to ALLOWED since I believe that better conveys they go inside an allow block.

  • ./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

@sagudev
Copy link
Member

sagudev commented Mar 24, 2024

I am wondering if it's worth adding complexity to codegen (which is already hard), just to make clippy happy or should we use more allows.

NOTE: This is not review, just me thinking out loud.

@eerii
Copy link
Contributor Author

eerii commented Mar 24, 2024

In some cases I'm thinking the same. It's hard to find the balance between clean generated code and clean generation code.

I'm also not sure if it's better to create abstractions or to keep the code mininal. For example, for the closure inside wrap_panic, some kind of closure generating wrapper could be used, replacing it in other parts of the code, but is this clearer or more obfuscated? Maybe the same can be done for the changes that deal with +/- 0.

I think I stride more towards solutions that create cleaner code such as the if/else simplification, but I'm not particularly married to any of them and would be happy to add allows for some instead.

@eerii
Copy link
Contributor Author

eerii commented Mar 24, 2024

Another unrelated question I had was regarding format strings. There are all types of formatting in the file (+, %, .format, f"", ...). Is there a preferred one? I tried to keep it consistent with what was already there. Maybe a separate pr could be made to unify the style of the file.

@sagudev
Copy link
Member

sagudev commented Mar 24, 2024

Another unrelated question I had was regarding format strings. There are all types of formatting in the file (+, %, .format, f"", ...). Is there a preferred one? I tried to keep it consistent with what was already there. Maybe a separate pr could be made to unify the style of the file.

This is because this script is old, before fstrings were a thing. I know that I introduced some of fstrings (if not all). I think python is generally moving towards fstrings and they are similar to what rust have, so I prefer those. Sometimes + is also useful sometimes. Maybe open a new issue so we can discuss this a bit more.

@eerii
Copy link
Contributor Author

eerii commented Mar 24, 2024

I think I also introduced some f strings in the last pr as well. I opened #31846 to discuss this!

@mrobinson mrobinson added this pull request to the merge queue Mar 25, 2024
Merged via the queue into servo:main with commit 9a76dd9 Mar 25, 2024
10 checks passed
ektuu pushed a commit to ektuu/servo that referenced this pull request Mar 25, 2024
* clippy: fix warnings in generated code

* clippy: fix wrap_panic closure warnings
@mukilan
Copy link
Member

mukilan commented Mar 26, 2024

Seems like this change has broken the nightly Linux build. Unlike the main build, we use Ubuntu 20.04 and Python 3.10 for nightly builds . Is this is an issue with code not being compatible with the python version?

@sagudev
Copy link
Member

sagudev commented Mar 26, 2024

Seems like this change has broken the nightly Linux build. Unlike the main build, we use Ubuntu 20.04 and Python 3.10 for nightly builds . Is this is an issue with code not being compatible with the python version?

Weird. str.removeprefix is from python3.9 according to https://docs.python.org/3/library/stdtypes.html#str.removeprefix. I suspect that we use py3.8 (default python in ubuntu 20.04) in codegen.

EDIT: Indeed we are:

fn find_python() -> String {
env::var("PYTHON3").ok().unwrap_or_else(|| {
let candidates = if cfg!(windows) {
["python3.8.exe", "python38.exe", "python.exe"]
} else {
["python3.8", "python3", "python"]
};
for &name in &candidates {
if Command::new(name)
.arg("--version")
.output()
.ok()
.map_or(false, |out| out.status.success())
{
return name.to_owned();
}
}
panic!(
"Can't find python (tried {})! Try fixing PATH or setting the PYTHON3 env var",
candidates.join(", ")
)
})
}

@sagudev sagudev mentioned this pull request Mar 26, 2024
5 tasks
ektuu pushed a commit to ektuu/servo that referenced this pull request Mar 28, 2024
* clippy: fix warnings in generated code

* clippy: fix wrap_panic closure warnings
@eerii eerii deleted the clippy_generated branch March 30, 2024 00:45
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

4 participants