ToSocketAddr trait and unification of network structures constructor methods #18462
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon. |
|
Excellent! It really bothered me that in hyper I was allocating a String from a SocketAddr just to have the connect method create a new one. |
| let port_str = try_opt!(parts_iter.next(), "invalid socket address"); | ||
| let host = try_opt!(parts_iter.next(), "invalid socket address"); | ||
| let port: u16 = try_opt!(FromStr::from_str(port_str), "invalid port value"); | ||
| resolve_socket_addr(host, port) |
There was a problem hiding this comment.
There's actually a FromStr implementation for SocketAddr which this should probably use instead
There was a problem hiding this comment.
Hm nevermind, I see what's going on here
|
I also want to add a little documentation on |
|
I've added some docs on |
|
This looks fantastic! I'm happy for this to land (needs a rebase, though.) |
This commit adds ToSocketAddr trait to std::io::net::ip module. This trait is used for generic conversion from different types (strings, (string, u16) tuples, etc.) into a SocketAddr instance. It supports multiple output SocketAddresses when it is appropriate (e.g. DNS name resolution). This trait is going to be used by TcpStream, TcpListener and UdpSocket structures.
TcpListener and TcpStream are converted to use ToSocketAddr trait in their constructor methods. [breaking-change]
UdpSocket constructor methods now use ToSocketAddr trait instead of SocketAddr. [breaking-change]
|
Rebased but still need to run tests to check that rebase was correct. |
|
Thanks again for this @netvl! |
bors
added a commit
that referenced
this pull request
Nov 5, 2014
This is a follow-up to [RFC PR #173](rust-lang/rfcs#173). I was told there that changes like this don't need to go through the RFC process, so I'm submitting this directly. This PR introduces `ToSocketAddr` trait as defined in said RFC. This trait defines a conversion from different types like `&str`, `(&str, u16)` or even `SocketAddr` to `SocketAddr`. Then this trait is used in all constructor methods for `TcpStream`, `TcpListener` and `UdpSocket`. This unifies these constructor methods - previously they were using different types of input parameters (TCP ones used `(&str, u16)` pair while UDP ones used `SocketAddr`), which is not consistent by itself and sometimes inconvenient - for example, when the address initially is available as `SocketAddr`, you still need to convert it to string to pass it to e.g. `TcpStream`. This is very prominently demonstrated by the unit tests for TCP functionality. This PR makes working with network objects much like with `Path`, which also uses similar trait to be able to be constructed from `&[u8]`, `Vec<u8>` and other `Path`s. This is a breaking change. If constant literals were used before, like this: ```rust TcpStream::connect("localhost", 12345) ``` then the nicest fix is to change it to this: ```rust TcpStream::connect("localhost:12345") ``` If variables were used before, like this: ```rust TcpStream::connect(some_address, some_port) ``` then the arguments should be wrapped in another set of parentheses: ```rust TcpStream::connect((some_address, some_port)) ``` `UdpSocket` usages won't break because its constructor method accepted `SocketAddr` which implements `ToSocketAddr`, so `bind()` calls: ```rust UdpSocket::bind(some_socket_addr) ``` will continue working as before. I haven't changed `UdpStream` constructor because it is deprecated anyway.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to RFC PR #173. I was told there that changes like this don't need to go through the RFC process, so I'm submitting this directly.
This PR introduces
ToSocketAddrtrait as defined in said RFC. This trait defines a conversion from different types like&str,(&str, u16)or evenSocketAddrtoSocketAddr. Then this trait is used in all constructor methods forTcpStream,TcpListenerandUdpSocket.This unifies these constructor methods - previously they were using different types of input parameters (TCP ones used
(&str, u16)pair while UDP ones usedSocketAddr), which is not consistent by itself and sometimes inconvenient - for example, when the address initially is available asSocketAddr, you still need to convert it to string to pass it to e.g.TcpStream. This is very prominently demonstrated by the unit tests for TCP functionality. This PR makes working with network objects much like withPath, which also uses similar trait to be able to be constructed from&[u8],Vec<u8>and otherPaths.This is a breaking change. If constant literals were used before, like this:
then the nicest fix is to change it to this:
If variables were used before, like this:
then the arguments should be wrapped in another set of parentheses:
UdpSocketusages won't break because its constructor method acceptedSocketAddrwhich implementsToSocketAddr, sobind()calls:will continue working as before.
I haven't changed
UdpStreamconstructor because it is deprecated anyway.