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

Fixing cargo clippy complaints #3448

Merged
merged 13 commits into from
Aug 16, 2022
Merged

Fixing cargo clippy complaints #3448

merged 13 commits into from
Aug 16, 2022

Conversation

franklee26
Copy link
Contributor

@franklee26 franklee26 commented Aug 15, 2022

Resolved issues:

Description of changes:

Fixed broken cargo clippy ci/cd job.

Call-outs:

Address any potentially confusing code. Is there code added that needs to be cleaned up later? Is there code that is missing because it’s still in development?

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@franklee26 franklee26 added the do_not_merge PR might needs something before merging, even if approved and passing label Aug 15, 2022
@franklee26 franklee26 changed the title Fixing cargo clippy complaints [IGNORE] Fixing cargo clippy complaints Aug 15, 2022
@franklee26 franklee26 changed the title [IGNORE] Fixing cargo clippy complaints Fixing cargo clippy complaints Aug 15, 2022
@franklee26 franklee26 added bindings/rust s2n-core team and removed do_not_merge PR might needs something before merging, even if approved and passing labels Aug 15, 2022
@franklee26 franklee26 marked this pull request as ready for review August 15, 2022 22:34
@lrstewart lrstewart requested a review from camshaft August 15, 2022 22:40
Comment on lines +201 to +202
let iter = functions.iter();
for (feature, function) in iter {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error:

> cargo clippy --manifest-path bindings/rust/Cargo.toml --all-targets -- -D warnings                        

    Checking generate v0.1.0 (/Volumes/workplace/s2n-tls/bindings/rust/generate)
    Checking s2n-tls v0.0.11 (/Volumes/workplace/s2n-tls/bindings/rust/s2n-tls)
error: temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression
   --> generate/src/main.rs:201:36
    |
201 |         for (feature, function) in functions.iter() {
    |                                    ^^^^^^^^^^^^^^^^
202 |             // don't generate tests for types
203 |             if types.contains(function) {
    |                ----- another value with significant `Drop` created here
...
208 |             if feature.is_some() && functions.contains(&(None, function.to_string())) {
    |                                     --------- another value with significant `Drop` created here
...
224 |         }
    |          - temporary lives until here
    |
    = note: `-D clippy::significant-drop-in-scrutinee` implied by `-D warnings`
    = note: this might lead to deadlocks or other unexpected behavior
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitant to even fix this. This lint is unhelpful and doesn't make the code any more readable. In fact, I would argue it makes it less readable since it introduces an unnecessary variable in scope. From my reading, it looks like they're going to disable it by default in the next release: rust-lang/rust-clippy#8987

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it doesn't seem helpful, but I don't think it's bad enough to bother suppressing. One unnecessary variable seems fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think an extra let declaration is ok, especially if it keeps clippy happy. When clippy disables this lint, we can remove this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

When clippy disables this lint, we can remove this change.

Not without a TODO and an issue or something we won't :) But I also don't think it's important enough to spend time reverting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah not a blocker. Especially since this code isn't even consumed outside of publishing. I'm just kind of annoyed with clippy pushing new warnings by default that are not universally applicable.

@franklee26 franklee26 requested a review from a team as a code owner August 16, 2022 16:43
.github/CODEOWNERS Outdated Show resolved Hide resolved
@franklee26 franklee26 enabled auto-merge (squash) August 16, 2022 20:05
@franklee26 franklee26 merged commit 625a51b into main Aug 16, 2022
@franklee26 franklee26 deleted the cargo_clippy_fix branch August 16, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants