Skip to content

chore: lint libc-test/build.rs #4412

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

Merged
merged 1 commit into from
Apr 25, 2025
Merged

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Apr 15, 2025

run cargo clippy --all-targets on libc-test/build.rs, and fix all default issues.

Notes

  • copy_dir_hotfix had a replace parameter that was never used

@rustbot label +stable-nominated

@rustbot
Copy link
Collaborator

rustbot commented Apr 15, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Apr 15, 2025
@nyurik nyurik force-pushed the libc-test-lints branch 7 times, most recently from 51658d2 to 4accf8f Compare April 15, 2025 18:41
@rustbot rustbot added the ctest Issues relating to the ctest crate label Apr 15, 2025
@nyurik

This comment was marked as outdated.

@nyurik nyurik marked this pull request as draft April 15, 2025 20:15
@nyurik nyurik force-pushed the libc-test-lints branch 2 times, most recently from 20de9a7 to e679015 Compare April 15, 2025 21:00
@nyurik nyurik marked this pull request as ready for review April 15, 2025 21:37
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Most changes here look fine to me, but I think replacing match within cfg closures should be dropped (e.g. volatile_items and skip_*) since it breaks consistency with the rest. Those lints can be ignored for the whole file.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

One last nit then LGTM, please squash

Comment on lines 3577 to 3578
cfg.skip_type(move |ty| match ty {
"stat64" | "sighandler_t" | "off64_t" => true,
_ => false,
});
cfg.skip_type(|ty| matches!(ty, "stat64" | "sighandler_t" | "off64_t"));

cfg.skip_field_type(move |struct_, field| match (struct_, field) {
("siginfo_t", "si_value") | ("stat", "st_size") | ("sigaction", "sa_u") => true,
_ => false,
cfg.skip_field_type(|struct_, field| {
matches!(
(struct_, field),
("siginfo_t", "si_value") | ("stat", "st_size") | ("sigaction", "sa_u")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you drop the changes to matches! here? It's easier and less conflict-y to append new types/fields in the current form, plus consistency.

@nyurik
Copy link
Contributor Author

nyurik commented Apr 25, 2025

thx, fixed

run `cargo clippy --all-targets` on `libc-test/build.rs`, and fix all default issues.

### Notes
* `copy_dir_hotfix` had a `replace` parameter that was never used
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Thank you!

@tgross35 tgross35 enabled auto-merge April 25, 2025 05:00
@tgross35 tgross35 added this pull request to the merge queue Apr 25, 2025
Merged via the queue into rust-lang:main with commit ec7da63 Apr 25, 2025
50 checks passed
@nyurik nyurik deleted the libc-test-lints branch April 25, 2025 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ctest Issues relating to the ctest crate S-waiting-on-author stable-nominated This PR should be considered for cherry-pick to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants