chrono icon indicating copy to clipboard operation
chrono copied to clipboard

Refactor formatting

Open pitdicker opened this issue 2 years ago • 1 comments
trafficstars

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.

pitdicker avatar Oct 01 '23 12:10 pitdicker

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.

codecov[bot] avatar Oct 01 '23 12:10 codecov[bot]

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_std compatible. 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_std compatibility.
  • 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.

pitdicker avatar Mar 09 '24 17:03 pitdicker

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 avatar Mar 09 '24 17:03 pitdicker

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.

pitdicker avatar Mar 09 '24 17:03 pitdicker

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

pitdicker avatar Mar 22 '24 12:03 pitdicker