Replace pretty_toa with num-format. #38
Conversation
Unlike pretty_toa, num-format can create a &str representation of a number (with thousands separators) without allocating. Creating this kind of string representation from a number only occurs at one place in inferno's code; so replacing pretty_toa with num-format is a tiny improvement, but it is an improvement nontheless as it avoids an unnecessary allocation (which occurs in a loop). Fixes jonhoo#29.
|
Your Windows CI identified a small bug in num-format (thanks!). It was implementing a trait unconditionally on a unix-only type (instead of putting it behind a #[cfg(unix)] flag). I’m publishing a bug fix update to num-format shortly and will update this code to use version 0.2.1 as opposed to 0.2.0 once it’s up. Then the Windows CI should pass. |
inferno's Windows CI identified a small bug in num-format 0.2.0. A trait was being implemented on a unix-only type unconditionally. num-format 0.2.1 fixes this bug; so updating the Cargo.lock should fix the issue. While I was at it I just ran `cargo update` in general; so a few other dependencies also got the minor versions bumped.
|
Alight ... So that wasn't my only Windows issue. |
|
Pinging @bcmyers: any update? |
|
Need a couple more days (Friday maybe?), but have been actively working on 0.3.0 of num-format. The changes needed for a new version of num-format have nothing to do with the part that would be used in this code (they relate to asking linux, windows, bsd.. for locale information, which is not needed here), but nevertheless I want to get that right before putting up another version on crates.io. Once it's up, making this PR work should be trivial. |
Codecov Report
@@ Coverage Diff @@
## master #38 +/- ##
=======================================
Coverage 81.32% 81.32%
=======================================
Files 9 9
Lines 889 889
=======================================
Hits 723 723
Misses 166 166
Continue to review full report at Codecov.
|
|
I just published a new version of num-format on crates.io, which fixes the issues I was having on Windows. This PR now incorporates those changes. Let me know if you have any questions / issues. |
|
That's an awful lot of new dependencies just for number formatting :/ This pulls in |
|
Yah, the dependency explosion is due to stuff needed to support asking your OS for its locale (which is not needed here). It's not as bad as it looks since only Windows is the problem (and when you build on Unix the Windows dependencies don't come in). That said, what I need to do is put all the OS stuff behind a feature flag or something; so if you don't need that functionality you don't bring in a bunch of unneeded dependencies. That would get rid of all these dependencies as it relates to inferno. Let me take a crack at that and get back to you. This change should be much shorter. Issue: bcmyers/num-format#14 |
Version 0.4.0 of num-format hides functionality that requires a ton of dependencies behind a feature flag so that inferno, which doesn't use that functionality, doesn't have to suffer the dependencies.
|
Dependencies cut down to just |
Unlike pretty_toa, num-format can create
a &str representation of a number (with
thousands separators) without allocating.
Creating this kind of string representation from
a number only occurs at one place in inferno's
code; so replacing pretty_toa with num-format
is a tiny improvement, but it is an improvement
nontheless as it avoids an unnecessary allocation
(which occurs in a loop).
Fixes #29.