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

TcpListener::bind/TcpStream::connect (&str, Int), &str and SocketAddr{...} overrides #14755

Closed

Conversation

thomaslee
Copy link
Contributor

cc @alexcrichton

In #13919 we changed TcpStream::connect to accept a host and a port parameter instead of a somewhat "uncomfortable" SocketAddr. There were some reasonable complaints following the merge that basically said sometimes you might want a SocketAddr for various different reasons, and that carting strings around everywhere might be painful in certain situations.

@huonw and @zargony suggested it might be better for TcpStream::connect and TcpListener::bind to use traits so that they can operate with various different types of arguments. I gave that a shot, and I think the results are really nice.

This change allows the following:

// (&str, Int)
let conn1 = try!(TcpStream::connect(("mozilla.org", 80)))

// &str (I think this is what @alexcrichton originally wanted the API to look like prior to #13919)
let conn2 = try!(TcpStream::connect("mozilla.org:80"))

// SocketAddr{...} (this is the old pre-#13919 API)
let conn3 = try!(TcpStream::connect(SocketAddr{ip: Ipv4Addr(63, 254, 215, 20), port: 80}))

It would also be trivial to add support for e.g. (IpAddr, Int) tuples too, but I didn't want to get too carried away :)

I think this PR needs some work before it gets merged. In particular, connect_timeout hasn't been modified, and I'm not sure that we want two distinct traits (ToBindSocketAddr and ToSocketAddrs) for very similar use-cases. Figure it's at a point where it'll probably drive some discussion, though.

#[doc(hidden)]
trait ToSocketAddrs {
fn to_socket_addrs(self) -> IoResult<Vec<SocketAddr>>;
}
Copy link
Member

Choose a reason for hiding this comment

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

While it seems that this trait should indeed be private, I don't think it should be tagged with doc(hidden). Knowing the implementors of this trait will be very important for discoverability to what you can pass to connect.

@alexcrichton
Copy link
Member

This looks quite interesting, thanks for working on this! If we go with this change, can you lift the relevant trait into the net module and use it with UDP as well?

@thomaslee
Copy link
Contributor Author

Absolutely, makes sense. I'll hold off on the other proposed changes here until I get a go-ahead. I think all the changes you're proposing here make sense, though I guess we'll want to figure out what to do with (&'a str, Int) etc.

@zargony
Copy link
Contributor

zargony commented Jun 10, 2014

I like this. I think it's important to be able to use connect and bind without the overhead of a string and running the parser. Nice work! :)

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

We've started to grow generics like BytesContainer in more locations, so I would think that this would be ok to merge if my comments were addressed.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2023
Fix: a TODO and some clippy fixes

- fix(todo): implement IntoIterator for ArenaMap<IDX, V>
- chore: remove unused method
- fix: remove useless `return`s
- fix: various clippy lints
- fix: simplify boolean test to a single negation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants