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

A few ergonomic From impls for SocketAddr/IpAddr #39372

Merged
merged 2 commits into from
Feb 8, 2017

Conversation

seanmonstar
Copy link
Contributor

My main motivation is removing things like this: "127.0.0.1:3000".parse().unwrap(). Instead, this now works: SocketAddr::from(([127, 0, 0, 1], 3000)) or even ([127, 0, 0, 1], 3000).into()) when passing to a function.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@nagisa
Copy link
Member

nagisa commented Jan 28, 2017

I wonder if something like

impl<T: Into<Ipv4Addr>> From<T> for IpAddr { ... }

would be at odds with the coherence.

@seanmonstar
Copy link
Contributor Author

@nagisa I considered it, but also worried about the possibility of some type T that could conceivable have both Into<Ipv4Addr> and Into<Ipv6Addr>, and if that would cause problems. It does seem highly unlikely to do that, but was still a thought, so this seemed more conservative. I can switch to that if consensus is that it is the better route.

@clarfonthey
Copy link
Contributor

For completion, can you add implementations for SocketAddrV[46]?

@seanmonstar
Copy link
Contributor Author

@clarcharr implementations for From<SocketAddrV4> and From<SocketAddrV6> already exist, starting in 1.16.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 30, 2017
@alexcrichton
Copy link
Member

I worry slightly that this seems "too clever" but we've got precedent elsewhere in the standard library for working with (&str, u16) as a "socket address" with a ToSocketAddrs impl. In that case this seems reasonable to me, so:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 30, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@aturon
Copy link
Member

aturon commented Jan 31, 2017

This is cool! 👍 from me.


#[stable(feature = "ip_from_slice", since = "1.17.0")]
impl From<[u16; 8]> for IpAddr {
fn from(octets: [u16; 8]) -> IpAddr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny correction: this should say segments instead of octects.

@alexcrichton
Copy link
Member

ping @BurntSushi, @brson (checkboxes)

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 7, 2017

📌 Commit cd603e4 has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 8, 2017
…crichton

A few ergonomic From impls for SocketAddr/IpAddr

My main motivation is removing things like this: `"127.0.0.1:3000".parse().unwrap()`. Instead, this now works: `SocketAddr::from(([127, 0, 0, 1], 3000))` or even `([127, 0, 0, 1], 3000).into())` when passing to a function.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 8, 2017
…crichton

A few ergonomic From impls for SocketAddr/IpAddr

My main motivation is removing things like this: `"127.0.0.1:3000".parse().unwrap()`. Instead, this now works: `SocketAddr::from(([127, 0, 0, 1], 3000))` or even `([127, 0, 0, 1], 3000).into())` when passing to a function.
bors added a commit that referenced this pull request Feb 8, 2017
Rollup of 13 pull requests

- Successful merges: #38764, #39361, #39372, #39374, #39400, #39426, #39431, #39459, #39482, #39545, #39593, #39620, #39621
- Failed merges:
@bors bors merged commit cd603e4 into rust-lang:master Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants