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 clippy lints #324

Merged
merged 16 commits into from
Jan 7, 2021
Merged

Fix clippy lints #324

merged 16 commits into from
Jan 7, 2021

Conversation

Sh3Rm4n
Copy link
Contributor

@Sh3Rm4n Sh3Rm4n commented Dec 31, 2020

No description provided.

@Sh3Rm4n Sh3Rm4n changed the title Test? Fix clippy lints Dec 31, 2020
@Sh3Rm4n
Copy link
Contributor Author

Sh3Rm4n commented Jan 3, 2021

Feel free to cherry-pick commits, if some changes are not desired.

Clippy was added to the CI to confirm the correct alliance of the suggested fixes.
I'll can remove or adjust this part, if you find clippy to annoying. (Maybe only show clippy erros and ignore clippy warnings?)

I'm thinking about adding a rustfmt ci job as well? The code base is mostly consistent with the default rustfmt style, but not everywhere.

I'm not sure about the UI tests failures. Is this directly related to my changes or where these introduced because of the 1.49 rustc release?

Copy link
Member

@japaric japaric 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 the PR. The clippy fixes look good to me. I'm OK with adding a clippy CI job. A rustfmt one would also be welcomed.

components: clippy
- uses: actions-rs/clippy-check@v1
with:
token: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

(why) is this needed? (also, I don't think we have set any secrets for this repo.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what the clippy-check action requires and suggest in its default configuration.

This step even fails, when this toke key is not set. I guess, it's about a hidden token generated by github, which has to be passed to clippy-check, so that it can annotate PRs with clippy warnings? TBH, I haven't looked much further. The correct answer can be probably found at the clippy-check repository.

@@ -2,4 +2,4 @@ error: `#[panic_handler]` attribute cannot be used together with `#[export_name]
--> $DIR/panic-handler-export-name.rs:5:1
|
5 | #[export_name = "hello"]
| ^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

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

you can undo these changes. I've fixed the UI issues in PR #325

#[warn(clippy::match_like_matches_macro)]
#[warn(clippy::single_char_add_str)]
Use of `unwrap_or` followed by a call to `new`
#[warn(clippy::ptr_arg)]
#[warn(clippy::single_char_pattern)]
#[warn(clippy::len_zero)]
#[warn(clippy::needless_lifetimes)]
#[warn(clippy::unnecessary_lazy_evaluations)]
#[warn(clippy::redundant_field_names)]
#[warn(clippy::collapsible_if)]
#[warn(clippy::needless_lifetimes)]
@Sh3Rm4n
Copy link
Contributor Author

Sh3Rm4n commented Jan 6, 2021

I've added rustfmt and rebased onto main, so that the UI tests are running again.

I stopped at #[deny(clippy::uninit_assumed_init)], where I do understand the incentive, but I don't know how to solve it, nor if a solution is even necessary. I'm too inexperienced to evaluate safety / undefined behavior issues.

Same goes for the #[warn(clippy::missing_safety_doc)] warnings. Though, these are warnings and could be technically ignored.

 #[warn(clippy::declare_interior_mutable_const)]
bors bot added a commit that referenced this pull request Jan 7, 2021
329: document safety of implementation detail functions r=japaric a=japaric

fixes some clippy lints

should unblock #324

Co-authored-by: Jorge Aparicio <[email protected]>
@japaric
Copy link
Member

japaric commented Jan 7, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 7, 2021

Build succeeded:

@bors bors bot merged commit 4e8e127 into knurling-rs:main Jan 7, 2021
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