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

SocketAddr and friends now correctly pad its content #72398

Merged
merged 3 commits into from
May 29, 2020

Conversation

Lucretiel
Copy link
Contributor

Currently, IpAddr and friends correctly respect formatting parameters when printing via Display. This PR makes SocketAddr and friends do the same thing.

@rust-highfive
Copy link
Collaborator

r? @hanna-kruppe

(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 May 20, 2020
@Mark-Simulacrum
Copy link
Member

The implementation here looks good to me. It's unclear to me though that we want to support padding in all of the various formatting impls; @rust-lang/libs, do we have a decision on that yet? (Both in the general sense and in the specific of these two impls).

@Lucretiel
Copy link
Contributor Author

It makes sense that we wouldn't want it in general, but imo it's a bit surprising that "192.168.0.1" formats with padding but "192.168.0.1:8080" does not. I'd argue that it should at least be consistent within a "family" of types.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Seems good to me.

It is not a goal to support padding in all impls, but where someone needs it, it's fine to add. In the case of SocketAddr it's easy to imagine a program that wants to print a table with a socket addr column and uses the formatter paddings to align columns.

src/libstd/net/addr.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

@bors r+

Approving in that case, I believe the code is correct.

@bors
Copy link
Contributor

bors commented May 22, 2020

📌 Commit a126445bcdab83f04401b0e91f1df8dae31e07c6 has been approved by Mark-Simulacrum

@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 May 22, 2020
@Lucretiel
Copy link
Contributor Author

Failures here seem related to today's github outage

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 28, 2020
…lacrum

Various minor improvements to Ipv6Addr::Display

Cleaned up `Ipv6Addr::Display`, especially with an eye towards simplifying and reducing duplicated logic. Also added a fast-path optimization, similar to rust-lang#72399 and rust-lang#72398.

- Defer to `Ipv4Addr::fmt` when printing an Ipv4 address
- Fast path: write directly to `f` without an intermediary buffer when there are no alignment options
- Simplify finding the inner zeroes-span
@bors
Copy link
Contributor

bors commented May 29, 2020

☔ The latest upstream changes (presumably #72716) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 29, 2020
@Lucretiel
Copy link
Contributor Author

Fixed

RalfJung added a commit to RalfJung/rust that referenced this pull request May 29, 2020
…lacrum

Various minor improvements to Ipv6Addr::Display

Cleaned up `Ipv6Addr::Display`, especially with an eye towards simplifying and reducing duplicated logic. Also added a fast-path optimization, similar to rust-lang#72399 and rust-lang#72398.

- Defer to `Ipv4Addr::fmt` when printing an Ipv4 address
- Fast path: write directly to `f` without an intermediary buffer when there are no alignment options
- Simplify finding the inner zeroes-span
@Amanieu
Copy link
Member

Amanieu commented May 29, 2020

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 29, 2020

📌 Commit 06a97a0 has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 29, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#72310 (Add Peekable::next_if)
 - rust-lang#72383 (Suggest using std::mem::drop function instead of explicit destructor call)
 - rust-lang#72398 (SocketAddr and friends now correctly pad its content)
 - rust-lang#72465 (Warn about unused captured variables)
 - rust-lang#72568 (Implement total_cmp for f32, f64)
 - rust-lang#72572 (Add some regression tests)
 - rust-lang#72591 (librustc_middle: Rename upvar_list to closure_captures)
 - rust-lang#72701 (Fix grammar in liballoc raw_vec)
 - rust-lang#72731 (Add missing empty line in E0619 explanation)

Failed merges:

r? @ghost
RalfJung added a commit to RalfJung/rust that referenced this pull request May 29, 2020
…lacrum

Various minor improvements to Ipv6Addr::Display

Cleaned up `Ipv6Addr::Display`, especially with an eye towards simplifying and reducing duplicated logic. Also added a fast-path optimization, similar to rust-lang#72399 and rust-lang#72398.

- Defer to `Ipv4Addr::fmt` when printing an Ipv4 address
- Fast path: write directly to `f` without an intermediary buffer when there are no alignment options
- Simplify finding the inner zeroes-span
@bors bors merged commit 8bce240 into rust-lang:master May 29, 2020
@dtolnay dtolnay self-assigned this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants