Skip to content

Rewrite <Duration as fmt::Display>::fmt() #25481

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Rewrite <Duration as fmt::Display>::fmt() #25481

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 149 additions & 15 deletions src/libstd/time/duration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,18 @@

use prelude::v1::*;

use cmp;
use fmt;
use io::{self, Cursor, SeekFrom};
use io::prelude::*;
use ops::{Add, Sub, Mul, Div};
use str;
use sys::time::SteadyTime;

const NANOS_PER_SEC: u32 = 1_000_000_000;
const NANOS_PER_MILLI: u32 = 1_000_000;
const MILLIS_PER_SEC: u64 = 1_000;
const NANOS_PER_MICRO: u32 = 1_000;

/// A duration type to represent a span of time, typically used for system
/// timeouts.
Expand Down Expand Up @@ -168,15 +173,114 @@ impl Div<u32> for Duration {

impl fmt::Display for Duration {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match (self.secs, self.nanos) {
(s, 0) => write!(f, "{}s", s),
(0, n) if n % NANOS_PER_MILLI == 0 => write!(f, "{}ms",
n / NANOS_PER_MILLI),
(0, n) if n % 1_000 == 0 => write!(f, "{}µs", n / 1_000),
(0, n) => write!(f, "{}ns", n),
(s, n) => write!(f, "{}.{}s", s,
format!("{:09}", n).trim_right_matches('0'))
// FIXME: fmt::Formatter::with_padding() isn't public, and the pad() function applies
// precision in a way we don't want. At such time as it becomes reasonable to support
// padding without reimplementing fmt::Formatter::with_padding() here, this should be
// updated to support padding. In preparation for that, we're already writing to a
// stack buffer.
Copy link
Member

Choose a reason for hiding this comment

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

Can you wrap this (and the remaining lines) to 100 characters? Although the style guide says 100 max it's more important to follow the style of the surrounding code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. You're asking me to wrap it to 100, which is already the limit it's wrapped at. Did you mean to ask for a different line length?

FWIW, I typically wrap doc comments at 79 but leave code at 100. I didn't wrap this to 79 because it's not a doc comment.

Copy link
Member

Choose a reason for hiding this comment

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

Oh oops sorry I meant wrap to 80!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried wrapping to 80, and the resulting code is significantly harder to read, because a bunch of lines that fit comfortably with 100 now have to be wrapped awkwardly. The code here is indented more than the rest of the module, so they already don't have as much space.

I also don't understand why you think it's important to wrap this to 80 characters. You say it's important to follow the style of the surrounding code, but I don't see any justification at all for that other than the fact that you wrote the surrounding code and you personally prefer 80 characters. If someone else had written this file, I suspect you wouldn't think it's so important. In fact, I know you wouldn't, because prior to 556e76b this file was written for a line limit of 100.

In general, following the style of the surrounding code is a sensible policy when the project does not have a well-defined style. And even when it does, it still makes sense with small changes to preserve the cohesive style. But rust-lang/rust has a (reasonably-)well-defined style, and this is a significant chunk of new functionality (as opposed to individual lines here and there). And since the readability of this code suffers significantly with the shorter line length, I don't think it makes sense to restrict it like that.


// µ (U+00B5) is 0xC2B5 in UTF-8.
// The following is >= the longest possible string we can format.
let mut buf = *b"18446744073709551615.999999999\xC2\xB5s";

fn write_to_buf<'a>(dur: &Duration, f: &fmt::Formatter, mut buf: &'a mut [u8])
Copy link
Member

Choose a reason for hiding this comment

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

This seems quite complicated, and perhaps not getting a lot of bang for our buck, would it be possible to use something simpler which perhaps has an intermediate buffer allocation? It's quite unlikely that the fmt::Display implementation for Duration is going to be at the middle of a tight loop, and otherwise this code is somewhat difficult to follow as there's a lot going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think using a buffer allocation would simplify it, that would just end up writing to a Vec<u8> instead of a Cursor<&mut [u8]>, but the code otherwise would be the same. The complexity here comes just from the fact that I'm writing to an intermediate buffer at all, which I want to preserve because I think we should support padding/alignment on all stdlib fmt::Display implementations. The fmt API doesn't currently expose the necessary tools to do this, but if nobody else deals with that, I'm planning on at some point trying to figure out the right way to fix that so we can support padding properly.

Some of the complexity also comes from the fact that the flt2dec code is all private. I'd love to be able to explicitly call into that code with integral values for the various components, but for now I'm using the hack documented in this code. I figure I'll take a look again in the future when our new float formatting code is in place.

I do agree that this is a bit complicated. It ended up being more complicated than I had first expected. Part of that is due to the need to round the decimal when not printing at full precision (this rounding is the main reason I fell back to using the float serializing).

-> io::Result<&'a str> {
fn backup_over_zeros<'a>(cursor: &mut Cursor<&'a mut [u8]>) {
let mut pos = cursor.position() as usize;
{
let buf = cursor.get_ref();
loop {
match buf[pos-1] {
b'0' => { pos -= 1; }
b'.' => { pos -= 1; break; }
_ => { break; }
}
}
}
cursor.set_position(pos as u64);
}

let mut cursor = Cursor::new(&mut buf[..]);
let precision = f.precision();
match (dur.secs, dur.nanos) {
(0, n) if n < NANOS_PER_MICRO => try!(write!(cursor, "{}ns", n)),
(s, 0) if precision.unwrap_or(0) == 0 => try!(write!(cursor, "{}s", s)),
(0, n) => {
let (unit, suffix, max_prec) = if n >= NANOS_PER_MILLI {
(NANOS_PER_MILLI, "ms", 6)
} else {
(NANOS_PER_MICRO, "µs", 3)
};
// Leverage our existing floating-point formatting implementation
let n = n as f64 / unit as f64;
if let Some(precision) = precision {
try!(write!(cursor, "{:.*}{}", cmp::min(precision, max_prec), n, suffix));
} else {
// variable precision up to 3 decimals
// just print all 3 decimals and then back up over zeroes.
try!(write!(cursor, "{:.3}", n));
backup_over_zeros(&mut cursor);
try!(write!(cursor, "{}", suffix));
}
}
(s, n) => {
try!(write!(cursor, "{}", s));
// we're going to cheat a little here and re-use the same float trick above.
// but because 0.1234 has a leading 0, we're going to back up a byte, save it,
// overwrite it, and then restore it.
let n = n as f64 / NANOS_PER_SEC as f64;
try!(cursor.seek(SeekFrom::Current(-1)));
let saved_pos = cursor.position() as usize;
let saved_digit = cursor.get_ref()[saved_pos];
if let Some(precision) = precision {
try!(write!(cursor, "{:.*}s", cmp::min(precision, 9), n));
} else {
// variable precision up to 3 decimals
try!(write!(cursor, "{:.3}", n));
backup_over_zeros(&mut cursor);
try!(write!(cursor, "s"));
}
// make sure we didn't round up to 1.0 when printing the float
if cursor.get_ref()[saved_pos] == b'1' {
// we did. back up and rewrite the seconds
if s == u64::max_value() {
// we can't represent a larger value. Luckily, we know that
// u64::max_value() ends with '5', so we can just replace it with '6'.
cursor.get_mut()[saved_pos] = b'6';
} else {
try!(cursor.seek(SeekFrom::Start(0)));
try!(write!(cursor, "{}", s+1));
match precision {
None | Some(0) => {},
Some(precision) => {
// we need to write out the trailing zeroes
try!(write!(cursor, "."));
for _ in 0..cmp::min(precision, 9) {
try!(write!(cursor, "0"));
}
}
}
try!(write!(cursor, "s"));
}
} else {
// restore the digit we overwrite earlier
cursor.get_mut()[saved_pos] = saved_digit;
}
}
}
// buf now contains our fully-formatted value
let pos = cursor.position() as usize;
let buf = &cursor.into_inner()[..pos];
// formatting always writes strings
Ok(unsafe { str::from_utf8_unchecked(buf) })
}

let result = write_to_buf(self, f, &mut buf);
// writing to the stack buffer should never fail
debug_assert!(result.is_ok(), "Error in <Duration as Display>::fmt: {:?}", result);
let buf_str = try!(result.or(Err(fmt::Error)));
// If fmt::Formatter::with_padding() was public, this is where we'd use it.
write!(f, "{}", buf_str)
Copy link
Member

Choose a reason for hiding this comment

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

This method is actually exposed as f.pad(buf_str), although another option here would also be to use buf_str.fmt(f)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually can't use f.pad(buf_str), because it interprets precision as the maximum width of the string to print, but we're using precision here for a different purpose. I'm actually kind of surprised the formatting code doesn't have any way of changing the flags, and I decided not to attempt to hack it with a write!() call that uses a caller-supplied width because that discards alignment and fill.

}
}

Expand Down Expand Up @@ -260,15 +364,45 @@ mod tests {

#[test]
fn display() {
assert_eq!(Duration::new(0, 1).to_string(), "1ns");
assert_eq!(Duration::new(0, 1_000).to_string(), "1µs");
assert_eq!(Duration::new(0, 1_000_000).to_string(), "1ms");
assert_eq!(Duration::new(1, 0).to_string(), "1s");
assert_eq!(Duration::new(0, 999).to_string(), "999ns");
assert_eq!(Duration::new(0, 1_001).to_string(), "1.001µs");
assert_eq!(Duration::new(0, 1_100).to_string(), "1.1µs");
assert_eq!(Duration::new(0, 1_234_567).to_string(), "1.235ms");
assert_eq!(Duration::new(1, 234_567_890).to_string(), "1.235s");
assert_eq!(Duration::new(1, 999_999).to_string(), "1.001s");

assert_eq!(Duration::new(0, 2).to_string(), "2ns");
assert_eq!(Duration::new(0, 2_000).to_string(), "2µs");
assert_eq!(Duration::new(0, 2_000_000).to_string(), "2ms");
assert_eq!(Duration::new(2, 0).to_string(), "2s");
assert_eq!(Duration::new(2, 2).to_string(), "2.000000002s");
assert_eq!(Duration::new(2, 2_000_000).to_string(),
"2.002s");
assert_eq!(Duration::new(0, 2_000_002).to_string(),
"2000002ns");
assert_eq!(Duration::new(2, 2_000_002).to_string(),
"2.002000002s");
assert_eq!(Duration::new(2, 2).to_string(), "2s");
assert_eq!(Duration::new(2, 2_000_000).to_string(), "2.002s");
assert_eq!(Duration::new(0, 2_000_002).to_string(), "2ms");
assert_eq!(Duration::new(2, 2_000_002).to_string(), "2.002s");
assert_eq!(Duration::new(0, 0).to_string(), "0ns");

assert_eq!(format!("{:.9}", Duration::new(2, 2)), "2.000000002s");
assert_eq!(format!("{:.6}", Duration::new(2, 2)), "2.000000s");
assert_eq!(format!("{:.6}", Duration::new(0, 2_000_002)), "2.000002ms");
assert_eq!(format!("{:.20}", Duration::new(1, 1)), "1.000000001s");
assert_eq!(format!("{:.3}", Duration::new(1, 0)), "1.000s");
assert_eq!(format!("{:.3}", Duration::new(0, 1)), "1ns");
assert_eq!(format!("{:.0}", Duration::new(0, 1_001)), "1µs");
assert_eq!(format!("{:.0}", Duration::new(0, 1_500)), "2µs");
assert_eq!(format!("{:.9}", Duration::new(0, 100)), "100ns");
assert_eq!(format!("{:.9}", Duration::new(0, 100_000)), "100.000µs");
assert_eq!(format!("{:.9}", Duration::new(0, 100_000_000)), "100.000000ms");
assert_eq!(format!("{:.9}", Duration::new(100,0)), "100.000000000s");

assert_eq!(format!("{:.9}", Duration::new(u64::max_value(), 999_999_999)),
"18446744073709551615.999999999s");
assert_eq!(Duration::new(u64::max_value(), 999_999_999).to_string(),
"18446744073709551616s");
assert_eq!(format!("{:.3}", Duration::new(u64::max_value(), 999_999_999)),
"18446744073709551616.000s")
}
}