Conversation
b21a0f7 to
fd03e01
Compare
src/lib.rs
Outdated
| f.pad(&to_string_format(self.0, Format::IEC)) | ||
| f.pad(&self.display().to_string()) |
There was a problem hiding this comment.
be nice to get rid of this allocation
fd03e01 to
190d56c
Compare
src/display.rs
Outdated
| let unit_prefixes = match self.format { | ||
| Format::Iec | Format::Short => crate::UNITS_IEC.as_bytes(), | ||
| Format::Si => crate::UNITS_SI.as_bytes(), | ||
| }; |
There was a problem hiding this comment.
Short is currently assumed to use IEC (binary) units due to the motivating use case of sort -h. WDYT about that?
There was a problem hiding this comment.
How about combination:
enum ByteUnit {
Iec,
Si,
}
enum ByteFormat {
/// Human readable output.
Human(ByteUnit),
/// For `sort -h` and friends.
Short(ByteUnit),
}There was a problem hiding this comment.
how we repr this internal isn't very important since we're exposing methods on Display
i've added a note on the iec_short method about compat with sort -h
d1877f8 to
0484052
Compare
0484052 to
5f599dc
Compare
|
The added display benchmark shows that the additional fast path branch is worth it: Table sorted by median ascending (lower is beter) |
a0319cc to
277aefe
Compare
| if f.width().is_none() && f.precision().is_none() { | ||
| // allocation-free fast path for when no formatting options are specified | ||
| write!(f, "{display}") | ||
| } else { | ||
| f.pad(&display.to_string()) | ||
| } |
There was a problem hiding this comment.
this fast path trick was taken from std::fmt::Formatter::pad
https://doc.rust-lang.org/1.84.0/src/core/fmt/mod.rs.html#1438-1442
277aefe to
61c2d9c
Compare
61c2d9c to
4725e58
Compare
|
bump @MrCroxx for review i'm keen to get v2 prepped after this |
MrCroxx
left a comment
There was a problem hiding this comment.
LGTM
Thank you for your contribution. The code is awesome. 🤩
I have no comments on the code. But may I ask where does the "short" format come from, which standard or similar library?
we discussed a bit here #14 but i'm open to changing the name "short" to something else |
waiting for #71
similar to #32