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.

Sign up for GitHub

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

Jump to bottom

Rewrite style.rs using syn #4220

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Rewrite style.rs using syn #4220

wants to merge 14 commits into from

Conversation

rmehri01
Copy link

@rmehri01 rmehri01 commented Dec 25, 2024 •
edited
Loading

Description

  • Use syn instead of text-based parsing since it's more accurate and easier to work with
  • Move style.rs to libc-test and make it run as a part of cargo test, as well as cleaning it up a bit and adding tests for the style checker implementation itself
  • Use a visitor that will walk over all the items in the file, including within cfg_if! macros, and tries to apply the same logic that the previous style.rs script did
  • Fix some lints that came up as a result of rewriting the script
  • Not quite sure what to do about the multiple s_macros yet but it seems like it should be easier since we have access to the attributes

Closes #4109

Sources

Checklist

  • Relevant tests in libc-test/semver have been updated
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI

@rustbot
Copy link
Collaborator

rustbot commented Dec 25, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@tgross35
Copy link
Contributor

Huge thanks for working on this!! I’ll take a look in a couple of days :)

@rmehri01 rmehri01 force-pushed the syn-style branch 2 times, most recently from 61d9576 to d54c31a Compare December 25, 2024 02:05
rmehri01
rmehri01 commented Dec 25, 2024
View reviewed changes
libc-test/test/style.rs Outdated Show resolved Hide resolved
libc-test/test/style.rs Outdated Show resolved Hide resolved
tgross35
tgross35 reviewed Dec 31, 2024
View reviewed changes
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.

Sorry that took a little longer than expected. Overall this looks great, most of the comments I left relate to things we can probably upgrade now since the original is from 2016.

One meta thing - maybe it's better to put this into a new module of libc-test/src/ and then only run it in libc-test/test. Then you could add a new test file to check this implementation itself, just have a bunch of examples in strings and assert they pass/fail.

libc-test/test/style.rs Outdated Show resolved Hide resolved
libc-test/test/style.rs Outdated Show resolved Hide resolved
libc-test/test/style.rs Outdated Show resolved Hide resolved
ci/style.sh Outdated Show resolved Hide resolved
libc-test/test/style.rs Outdated Show resolved Hide resolved
libc-test/test/style.rs Outdated Show resolved Hide resolved
libc-test/test/style.rs Outdated Show resolved Hide resolved
libc-test/test/style.rs Outdated Show resolved Hide resolved
libc-test/test/style.rs Outdated Show resolved Hide resolved
libc-test/test/style.rs Outdated Show resolved Hide resolved
@rmehri01 rmehri01 force-pushed the syn-style branch 2 times, most recently from ed1c65d to 822170d Compare January 2, 2025 03:29
@rmehri01
Copy link
Author

rmehri01 commented Jan 2, 2025

One meta thing - maybe it's better to put this into a new module of libc-test/src/ and then only run it in libc-test/test. Then you could add a new test file to check this implementation itself, just have a bunch of examples in strings and assert they pass/fail.

I couldn't quite get it to work with libc-test/src/ since there were some linking errors, I think having to do with using cc in the build script. For now I just moved the common functionality into tests/style/mod.rs similar to what is suggested here so that we can test the style checker implementation as well: https://doc.rust-lang.org/rust-by-example/testing/integration_testing.html.

@rmehri01 rmehri01 marked this pull request as ready for review January 2, 2025 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 left review comments

Assignees

@tgross35 tgross35

Labels
O-linux O-unix O-wasi O-x86 S-waiting-on-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rewrite ci/style.rs using syn
3 participants
@rmehri01 @rustbot @tgross35