chrono
chrono copied to clipboard
Refactor formatting
We currently have a 234-line format_inner function.
In https://github.com/chronotope/chrono/pull/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 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 :warning: |
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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:
- #1196 was a large rewrite of our formatting code. Parts of it are performance improvements and making the code
no_stdcompatible. The goal is to make it easier to re-use parsed format strings, and to avoid panics while formatting. - #1163 was split out as a first step, including only the performance improvements and
no_stdcompatibility. - This PR is split out from #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.
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.
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 :smile:. That's doable.
@djc Do you want to give this a second look?