RFC: Add SocketAddr conversion trait to std::io::net and generalize socket constructors #173
Conversation
|
I'd be willing to implement it if it is accepted. |
|
|
||
| ```rust | ||
| pub trait AsSocketAddr { | ||
| fn as_socket_addr(&self) -> IoResult<SocketAddr>; |
There was a problem hiding this comment.
For the get_host_addresses use case, this will likely want to return IoResult<Vec<SocketAddr>> so the TcpStream::connect layer can choose between the addresses it receives.
There was a problem hiding this comment.
In that case, I think, it makes sense to add default method to the trait which returns the head of the vector, while this new method returning a vector becomes the abstract one. I'll add this to the RFC.
Added `as_socket_addr_all()` method and explained its function and relationship with its counterpart.
|
|
|
@erickt, thanks for the link. But I'm not quite sure if we want to use move semantics here. |
|
@netvl: can you explain why? |
|
@erickt, well, passing by reference should is usually a default, no? It is the most flexible way. And for default implementations there will be no "unwrapping" of any kind, so move semantics do not give anything of value here. |
|
A quote from the link you provided:
Here we obviously are not decreasing any abstraction; host names and IP address are different things, and neither is contained in another. I think |
|
As one of the original authors of that API, I'm not sure why it was ever changed from just The API was supposed to be a core set of primitives with clear costs for making TCP connections. I'm all for the convenience of a function that auto-resolves if necessary, but such a higher-level convenience wrapper function should be part of a higher-level convenience API instead of the low-level core primitives. That said, I would suggest the following alternative:
Under that proposal we'd see something more like: which makes it clear that some additional cost (parsing + perhaps address resolution) is imposed compared to: which only constructs a connection. |
alexcrichton
force-pushed
the
master
branch
from
6357402
to
e0acdf4
Compare
|
Thanks for taking the time to write up this RFC! Changes such as this which don't have much to do with the language and are more geared towards portions of a library don't need to go through the RFC process, however. I'm going to close this for now in light of that. |
|
@alexcrichton, so I just can submit a PR? |
|
Yes |
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.
Rendered view