-
Notifications
You must be signed in to change notification settings - Fork 92
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 a bunch of clippy lints #183
base: master
Are you sure you want to change the base?
Conversation
Right, so about the last remaining lints, they fall in the following categories:
|
The largest reason this isn't some more specific newtype ('do not leak any failure cause that might be weaponized to the client') is backwards compatibility. It was probably a mistake not to use a more specifically named newtype instead. If clippy lints, it should be fixed with the next major version. Does clippy accept some name type aliases instead?
This would be a one-liner to change all the definitions in another PR. |
This might be a matter of accepting whether it should be thought of as a 'default'. The other choice here is providing a more descriptive name to the constructor, I tend to agree with clippy that |
Bordering controversy, but the state machines shouldn't be changed. Semantically they should be written like coroutines / generators which just isn't possible right now. I don't think that clippy lints when coroutines consume more space in one particular await than others. This isn't ideal but note that the state is usually stack-allocated anyways and barely moved. Changing the implementation's types or code flow for this lint would at best complicate the readability of a securitiy-critical part, the gains would need to be overwhelming to be convincing imo. It shouldn't really matter greatly for performance (I can be convinced with measurement if you think doing one is worth it). Maybe this should be added to the documentation. What are the instances on error types (and probably the |
Oh, it's usually not about changing any paths. It's usually just about boxing a variant of the enum, thereby reducing the stack allocated size, improving performance on But yeah, if they aren't moved a lot, then that's not relevant. As for the error, I'll post the exact ones it complains about later. |
The only one Clippy seems to have a problem with is
|
This PR has no real public changes, more of a generic fixing of clippy lints.
I just noticed a bunch of warnings since my RA install runs
cargo clippy
instead ofcargo check
.Thought it might be a few nice fixes, especially reducing the complexity of some function signatures by removing unnecessary explicit lifetimes.
I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to chose either at their option.