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

Add clippy suggestions #6203

Merged
merged 3 commits into from
Apr 17, 2023

Conversation

kevaundray
Copy link
Contributor

@kevaundray kevaundray commented Apr 12, 2023

Made a few clippy fixes in this PR. I did not do all of the clippy fixes as it may be that only certain things should be changed and or clippy fixes are not deemed necessary.

This PR is more of a temperature check :)

@kevaundray kevaundray requested review from a team as code owners April 12, 2023 18:16
@kevaundray kevaundray requested review from jameysharp and removed request for a team April 12, 2023 18:16
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. isle Related to the ISLE domain-specific language labels Apr 12, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "cranelift:meta", "isle"

Thus the following users have been cc'd because of the following labels:

  • cfallin: isle
  • fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

In general we don't use Clippy because there's a general consensus that it's too opinionated. That said, I like most of these changes!

There are a few that I would like for you to remove from this PR, and ideally arrange that Clippy will ignore them in the future. (I assume you can add something to the top-level clippy.toml but I haven't looked into Clippy's configuration.)

With these changes, I'd be happy to merge this. Thanks!

cranelift/codegen/meta/src/cdsl/types.rs Outdated Show resolved Hide resolved
cranelift/codegen/meta/src/srcgen.rs Outdated Show resolved Hide resolved
cranelift/isle/isle/src/ast.rs Outdated Show resolved Hide resolved
cranelift/isle/isle/src/error.rs Outdated Show resolved Hide resolved
cranelift/isle/isle/src/parser.rs Outdated Show resolved Hide resolved
cranelift/isle/isle/src/sema.rs Outdated Show resolved Hide resolved
cranelift/isle/isle/src/sema.rs Outdated Show resolved Hide resolved
cranelift/isle/isle/src/sema.rs Outdated Show resolved Hide resolved
cranelift/isle/isle/src/sema.rs Outdated Show resolved Hide resolved
cranelift/isle/isle/src/sema.rs Outdated Show resolved Hide resolved
@kevaundray
Copy link
Contributor Author

Thanks for the review! It seems that we can't use Clippy.toml to enable lints globally.

There was a workaround done by Embark, which is to put it in the .config/Cargo.toml file -- I can open another issue for this, as it would require removing the lints from each lib.rs file and putting them in .config/Cargo.toml

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

Yeah, let's not bother with trying to configure Clippy then. We really don't want to spend much time on Clippy lints.

That said, I'll go ahead and take these changes. I do think they are improvements.

I'm sure there are more improvements we could find by digging through Clippy's suggestions, but that isn't something we want to spend review time on.

We have a bunch of open issues that we'd love your help with. Take a look at cranelift:E-easy Issues suitable for newcomers to investigate, including Rust newcomers! and see if any of those sounds interesting to you!

@jameysharp jameysharp added this pull request to the merge queue Apr 17, 2023
Merged via the queue into bytecodealliance:main with commit 85118c8 Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants