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

Fix issues reported by clippy #1336

Merged
merged 3 commits into from
Sep 11, 2022
Merged

Fix issues reported by clippy #1336

merged 3 commits into from
Sep 11, 2022

Conversation

neunenak
Copy link
Contributor

@neunenak neunenak commented Sep 8, 2022

Fixes every issue reported by running vanilla-configuration cargo clippy.

Running clippy against just, the vast majority of the errors are:

```
error: the `Err`-variant returned from this function is very large
 --> src/unresolved_recipe.rs:9:8
  |
9 |   ) -> CompileResult<'src, Recipe<'src>> {
  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 144 bytes
  |
  = help: try reducing the size of `compile_error::CompileError<'src>`, for example by boxing large elements or replacing it with `Box<compile_error::CompileError<'src>>`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
```

The large size of `CompileResult` is mostly caused by `CompileErrorKind`
being a large type within `CompileError`. So, Box-ing that type on
`CompileError` solves the `CompileResult` problem without requiring too
many additional code changes.

Most of the time, just will only be compiling one value of type
`CompileResult`, which will hopefully most of the time not even have the
error variant on it. So Boxing (part of) that variant in order to reduce
the size of `CompileResult`, which will get passed between many
different functions, makes sense.
@casey
Copy link
Owner

casey commented Sep 11, 2022

How can I get these errors? When I run cargo clippy it doesn't complain.

@neunenak
Copy link
Contributor Author

Huh, I just realized, I had my default toolchain set to nightly, not stable. It looks like these are all lints that are only reported on nightly clippy.

@neunenak
Copy link
Contributor Author

I just tested it and this branch compiles and passes all tests on stable as well.

@casey
Copy link
Owner

casey commented Sep 11, 2022

Okay, sweet. I was worried I had something weird going on with the config.

@casey casey closed this Sep 11, 2022
@casey casey reopened this Sep 11, 2022
@casey
Copy link
Owner

casey commented Sep 11, 2022

Actually, looking at these, they all look like reasonable fixes, so why not merge them.

@casey casey enabled auto-merge (squash) September 11, 2022 08:37
@casey casey merged commit 7b7efca into casey:master Sep 11, 2022
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.

2 participants