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

Rewrite ci/style.rs using syn #4109

Open
tgross35 opened this issue Nov 19, 2024 · 2 comments
Open

Rewrite ci/style.rs using syn #4109

tgross35 opened this issue Nov 19, 2024 · 2 comments
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. E-medium E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.

Comments

@tgross35
Copy link
Contributor

The style checking script has been about the same since it was introduced in 2015, but its text-based parsing seems to make it get in the way a lot of the time.

Ideally it would be rewritten using syn, which should be more accurate and not have problems e.g. handling newlines or recognizing when we are within cfg_if. It could become part of the libc-test crate, so cargo run libc-test checks style.

@tgross35 tgross35 added E-medium E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-help-wanted Call for participation: Help is requested to fix this issue. labels Nov 19, 2024
@tgross35
Copy link
Contributor Author

One of the problems that should be fixed is that currently style.rs forbids >1 s! macro in the file. Usually this makes sense but we should be able to allow >1 if annotated with a #[cfg(not...)] (specifically not) that is unique to the file. Examples:

Accept this:

// Main `s!`
s! { /* ... */ }

// These are not supported on a single arch. It doesn't make sense to
// duplicate `foo` into every single file except one, so allow this here.
#[cfg(not(target_arch = "foo"))]
s! { struct foo { /* ... */ } }

// Similar to the above, no problems here
#[cfg(not(target_os = "illumos"))]
s! { struct bar { /* ... */ } }

Reject these:

// Main `s!`
s! { /* ... */ }

// A positive (no `not`) config: reject because this should go into
// the relevant file.
#[cfg(target_arch = "foo")]
s! { struct foo { /* ... */ } }
// Main `s!`
s! { /* ... */ }

// Similar to the first (accept) example
#[cfg(not(target_arch = "foo"))]
s! { struct foo { /* ... */ } }

// Reject this: same `cfg` as above, should be combined
#[cfg(not(target_arch = "foo"))]
s! { struct bar { /* ... */ } }

If anyone has any interest in picking this up, I'm happy to help.

@tgross35
Copy link
Contributor Author

Hm, maybe it doesn't need to be limited to only not config, it seems like it would still be nicer to have #[cfg(any(target_arch = "foo", target_arch = "bar", target_arch = "baz"))] than to duplicate the same struct in three different files. Guess that can take some thought once a rewrite is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. E-medium E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.
Projects
None yet
Development

No branches or pull requests

1 participant