Skip to content
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.

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

Make Duration respect width when formatting using Debug #88999

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

michiel-de-muynck
Copy link
Contributor

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

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
Collaborator

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
Copy link
Contributor

oli-obk commented Sep 17, 2021

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
Copy link
Contributor

bors commented Sep 17, 2021

📌 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
Copy link
Contributor

oli-obk commented Sep 17, 2021

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
Copy link
Contributor

bors commented Sep 22, 2021

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

@bors
Copy link
Contributor

bors commented Sep 22, 2021

💔 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
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
Contributor Author

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

@oli-obk
Copy link
Contributor

oli-obk commented Sep 23, 2021

@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
Copy link
Contributor

bors commented Sep 24, 2021

⌛ Testing commit 77ceb2b with merge f06f9bb...

@bors
Copy link
Contributor

bors commented Sep 24, 2021

☀️ 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
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