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 in Regressions. #1403

Merged

Conversation

YoshikiTakashima
Copy link
Contributor

Description of changes:

No change to kani behavior. Added clippy linter to regression suite.

Resolved issues:

Lack of clippy lint checking in kani regression script. #1373 will stay open to track actual clippy errors.

Call-outs:

The current setup addresses the following clippy regressions.

  • New clippy error/warning in a library that did not have them (thus not ignored)
  • New class of clippy warnings/errors by code change

Testing:

  • How is this change tested? Run the regression script

  • Is this a refactor change?

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

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

@YoshikiTakashima YoshikiTakashima requested a review from a team as a code owner July 23, 2022 01:56
@YoshikiTakashima YoshikiTakashima changed the title Yoshi clippy compliance Clippy in Regressions. Jul 25, 2022
Copy link
Contributor

@giltho giltho left a comment

Choose a reason for hiding this comment

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

I'm not sure how clippy gets configured for a whole workspace, so I'm not sure if the first comment has a solution, the second one should probably be "fixed" before merging

.github/workflows/format-check.yml Outdated Show resolved Hide resolved
scripts/setup/macos-10.15/install_deps.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I have 2 questions:

  1. Is there a reason why you preferred .cargo/config.toml instead of clippy.toml.
  2. How long does cargo clippy takes? I was wondering if we should enable this in our regression.

@YoshikiTakashima
Copy link
Contributor Author

YoshikiTakashima commented Jul 25, 2022

@celinval

  1. Is there a reason why you preferred .cargo/config.toml instead of clippy.toml.

I don't think that is possible, and keeping it so seems to be the stance from clippy devs: rust-lang/rust-clippy#1313 (comment). The idea behind it seems to be that you would control all lints at once, not just clippy lints.

  1. How long does cargo clippy takes? I was wondering if we should enable this in our regression.

It takes no longer than 2 compiles with cargo build --workspace. This is the first job to finish.

@giltho
Copy link
Contributor

giltho commented Jul 25, 2022

LGTM ✅

@YoshikiTakashima
Copy link
Contributor Author

Not sure how big the "code owner" set is but it still saids it waiting for review. @tedinski / @celinval

.cargo/config.toml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
scripts/setup/ubuntu/install_deps.sh Outdated Show resolved Hide resolved
@YoshikiTakashima YoshikiTakashima merged commit 542f7c4 into model-checking:main Jul 26, 2022
@YoshikiTakashima YoshikiTakashima deleted the yoshi-clippy-compliance branch July 26, 2022 18:51
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