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

Refactor formatting #1335

Merged
merged 7 commits into from
Apr 4, 2024
Merged

Refactor formatting #1335

merged 7 commits into from
Apr 4, 2024

Conversation

pitdicker
Copy link
Collaborator

We currently have a 234-line format_inner function.
In #1163 I found a way to make it a bit nicer and faster.
This is only the initial refactoring, that splits it up into three methods on DelayedFormat.

Every commit is as close to a copy-paste as compiles and passes rustfmt.

@codecov
Copy link

codecov bot commented Oct 1, 2023

Codecov Report

Attention: Patch coverage is 94.08602% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 91.78%. Comparing base (ffe2745) to head (9fb956e).

Files Patch % Lines
src/format/formatting.rs 94.08% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1335      +/-   ##
==========================================
- Coverage   91.79%   91.78%   -0.01%     
==========================================
  Files          37       37              
  Lines       18175    18167       -8     
==========================================
- Hits        16683    16674       -9     
- Misses       1492     1493       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Zomtir Zomtir left a comment

Choose a reason for hiding this comment

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

Looks sane. Reviewing this is quite something, mostly because the huge indentation and lack of encapsulation of the old code. So this patch is surely a welcome improvement.

I know that the original format_inner function used a simple w as the main parameter name. By the time I arrived at the third patch I already forgot and had to go back. Maybe write would be more verbose for people with a light memory like me.

Same goes for for Some(v). Still trying to figure out what it stands for... a padding value?

There were a few memory allocation changes from pass-by-reference to pass-by-copy (and back). I didn't track those through-full, but in the compiler I trust.

@pitdicker
Copy link
Collaborator Author

Thank you for reviewing!

There were a few memory allocation changes from pass-by-reference to pass-by-copy (and back). I didn't track those through-full, but in the compiler I trust.

Yes, this is just a copy-paste and minimal changes from by-reference to by-value as needed to keep the code compiling.

v is short for value. It is used within format_numeric for whatever value needs to be formatted as an integer.

For context:

  • New formatting API #1196 was a large rewrite of our formatting code. Parts of it are performance improvements and making the code no_std compatible. The goal is to make it easier to re-use parsed format strings, and to avoid panics while formatting.
  • New Formatter to replace DelayedFormat #1163 was split out as a first step, including only the performance improvements and no_std compatibility.
  • This PR is split out from New Formatter to replace DelayedFormat #1163 as just the most basic refactor.
  • I have another branch to build on this one and port over the performance improvements.

I hope this PR can land before it rots and needs to be redone from 0 to keep it a copy-paste. Hmmm, already has a conflict, I'll have to see about that.

@pitdicker
Copy link
Collaborator Author

Looks sane. Reviewing this is quite something, mostly because the huge indentation and lack of encapsulation of the old code. So this patch is surely a welcome improvement.

Rustfmt kept changing the formatting of this huge method. So part of the reason to split it over multiple methods is to avoid that.

@pitdicker
Copy link
Collaborator Author

I hope this PR can land before it rots and needs to be redone from 0 to keep it a copy-paste. Hmmm, already has a conflict, I'll have to see about that.

My own fault with #1473 😄. That's doable.

src/format/formatting.rs Outdated Show resolved Hide resolved
src/format/formatting.rs Outdated Show resolved Hide resolved
@pitdicker
Copy link
Collaborator Author

@djc Do you want to give this a second look?

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Looks great!

src/format/formatting.rs Outdated Show resolved Hide resolved
@pitdicker pitdicker merged commit fe43396 into chronotope:main Apr 4, 2024
35 checks passed
@pitdicker pitdicker deleted the formatting_optim branch April 4, 2024 05:26
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.

None yet

3 participants