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

Add a custom source code formatter to replace rust-fmt #303

Merged
merged 2 commits into from
Jun 25, 2023

Conversation

cuddlefishie
Copy link
Contributor

@cuddlefishie cuddlefishie commented Jun 8, 2023

Generated code looks very messy when not formatted,
rustfmt take a lot of time to format the code, so this crate
implements a reduced formatter that's supposed to run fast enough
that it can be part of the regular build process.

TODOs:

  • change StopWatch so the timing summary makes sense again
  • more "hard numbers" for the performance of the new formatter (MB/s?)

@bluenote10
Copy link
Contributor

Just wondering, would prettyplease fit in here?

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-303

@Bromeon
Copy link
Member

Bromeon commented Jun 9, 2023

Just wondering, would prettyplease fit in here?

We considered it, but it takes a syn syntax tree, not TokenStream -- so we'd need to add a heavy dependency and do full-blown Rust parsing. We also looked at rust_format, but from what I can see, it's mostly an abstraction around rustfmt or prettyplease.

@Bromeon Bromeon added feature Adds functionality to the library c: tooling CI, automation, tools labels Jun 9, 2023
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! The output code looks really much nicer 🙂

The test-cases directory is mostly for benchmarking, right? Do the 5 files differ substantially, or could we do with just 3 or so? (Asking because they add 0.5 MB to the repo forever; even if compressed in Git).

Also, I don't see the formatting time output in *stats.txt files, would it be possible to include that again?

godot-codegen/src/lib.rs Outdated Show resolved Hide resolved
godot-codegen/src/lib.rs Outdated Show resolved Hide resolved
godot-fmt/src/lib.rs Outdated Show resolved Hide resolved
godot-fmt/src/lib.rs Outdated Show resolved Hide resolved
godot-fmt/Cargo.toml Outdated Show resolved Hide resolved
godot-fmt/benches/gdext_tests.rs Outdated Show resolved Hide resolved
godot-fmt/src/lib.rs Outdated Show resolved Hide resolved
godot-fmt/src/lib.rs Outdated Show resolved Hide resolved
godot-fmt/benches/gdext_tests.rs Outdated Show resolved Hide resolved
godot-fmt/src/lib.rs Outdated Show resolved Hide resolved
Generated code looks very messy when not formatted,
`rustfmt` take a lot of time to format the code, so this crate
implements a reduced formatter that's supposed to run fast enough
that it can be part of the regular build process.
@cuddlefishie
Copy link
Contributor Author

I addressed the review comments! Thanks!

Because of the way StopWatch is set up and the way the SubmitFn is performing the formatting (or not) immediately, it's harder to get direct timing.

Comparing the total time in codegen-stats.txt with and without formatting gave me inconsistent results. Sometimes running with formatting was faster (?!), sometimes the other way around. So if somebody else can test this, that would be good :)

@cuddlefishie cuddlefishie marked this pull request as ready for review June 25, 2023 19:51
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this! The benchmarking and StopWatch measurements can be addressed at a later time.

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 25, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 6ba5b01 into godot-rust:master Jun 25, 2023
7 checks passed
@cuddlefishie cuddlefishie deleted the formatter branch June 25, 2023 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: tooling CI, automation, tools feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants