Skip to content

Make Duration respect width when formatting using Debug - #88999

Merged
bors merged 1 commit into
rust-lang:masterfrom
michiel-de-muynck:master
Sep 24, 2021
Merged

Make Duration respect width when formatting using Debug#88999
bors merged 1 commit into
rust-lang:masterfrom
michiel-de-muynck:master

Conversation

@michiel-de-muynck

@michiel-de-muynck michiel-de-muynck commented Sep 16, 2021

Copy link
Copy Markdown
Contributor

When printing or writing a std::time::Duration using Debug formatting, it previously completely ignored any specified width. This is unlike types like integers and floats, which do pad to width, for both Display and Debug, though not all types consider width in their Debug output (see e.g. #30164). Curiously, Duration's Debug formatting did consider precision.

This PR makes Duration pad to width just like integers and floats, so that

format!("|{:8?}|", Duration::from_millis(1234))

returns

|1.234s  |

Before you ask "who formats Debug output?", note that Duration doesn't actually implement Display, so Debug is currently the only way to format Durations. I think that's wrong, and Duration should get a Display implementation, but in the meantime there's no harm in making the Debug formatting respect width rather than ignore it.

I chose the default alignment to be left-aligned. The general rule Rust uses is: numeric types are right-aligned by default, non-numeric types left-aligned. It wasn't clear to me whether Duration is a numeric type or not. The fact that a formatted Duration can end with suffixes of variable length ("s", "ms", "µs", etc.) made me lean towards left-alignment, but it would be trivial to change it.

Fixes issue #88059.

Duration's Debug formatting previously ignored the width parameter.
This commit fixes that.

Fixes issue rust-lang#88059.
@rust-highfive

Copy link
Copy Markdown
Contributor

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2021
@michiel-de-muynck michiel-de-muynck changed the title Make Duration handle width when formatting using Debug Make Duration respect width when formatting using Debug Sep 16, 2021
@oli-obk

oli-obk commented Sep 17, 2021

Copy link
Copy Markdown
Contributor

The fact that a formatted Duration can end with suffixes of variable length ("s", "ms", "µs", etc.) made me lean towards left-alignment, but it would be trivial to change it.

Considering the fact that booleans are also default-left-aligned, that seems like a safe choice. We can wait for someone with strong opinions on this to give us a reason to change it.

@bors r+

Thanks! I just ran into such oddities earlier this week, and you're already fixing this one, great!

@bors

bors commented Sep 17, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 77ceb2b has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2021
@oli-obk

oli-obk commented Sep 17, 2021

Copy link
Copy Markdown
Contributor

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned dtolnay Sep 17, 2021
@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 21, 2021
the8472 added a commit to the8472/rust that referenced this pull request Sep 22, 2021
Make `Duration` respect `width` when formatting using `Debug`

When printing or writing a `std::time::Duration` using `Debug` formatting, it previously completely ignored any specified `width`. This is unlike types like integers and floats, which do pad to `width`, for both `Display` and `Debug`, though not all types consider `width` in their `Debug` output (see e.g. rust-lang#30164). Curiously, `Duration`'s `Debug` formatting *did* consider `precision`.

This PR makes `Duration` pad to `width` just like integers and floats, so that
```rust
format!("|{:8?}|", Duration::from_millis(1234))
```
returns
```
|1.234s  |
```

Before you ask "who formats `Debug` output?", note that `Duration` doesn't actually implement `Display`, so `Debug` is currently the only way to format `Duration`s. I think that's wrong, and `Duration` should get a `Display` implementation, but in the meantime there's no harm in making the `Debug` formatting respect `width` rather than ignore it.

I chose the default alignment to be left-aligned. The general rule Rust uses is: numeric types are right-aligned by default, non-numeric types left-aligned. It wasn't clear to me whether `Duration` is a numeric type or not. The fact that a formatted `Duration` can end with suffixes of variable length (`"s"`, `"ms"`, `"µs"`, etc.) made me lean towards left-alignment, but it would be trivial to change it.

Fixes issue rust-lang#88059.
@bors

bors commented Sep 22, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 77ceb2b with merge 94ebf7000eb7eb645ced41792cdc1f83c0d7b7af...

@bors

bors commented Sep 22, 2021

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 22, 2021
@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@michiel-de-muynck

Copy link
Copy Markdown
Contributor Author

I don't understand what went wrong. Looks like the build was cancelled?

@oli-obk

oli-obk commented Sep 23, 2021

Copy link
Copy Markdown
Contributor

@bors retry

sometimes bors has hickups

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2021
@bors

bors commented Sep 24, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 77ceb2b with merge f06f9bb...

@bors

bors commented Sep 24, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing f06f9bb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 24, 2021
@bors
bors merged commit f06f9bb into rust-lang:master Sep 24, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 24, 2021
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (f06f9bb): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -1.0% on full builds of deeply-nested-async)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants