TcpListener::bind/TcpStream::connect (&str, Int), &str and SocketAddr{...} overrides#14755
TcpListener::bind/TcpStream::connect (&str, Int), &str and SocketAddr{...} overrides#14755thomaslee wants to merge 2 commits intorust-lang:masterfrom
Conversation
There was a problem hiding this comment.
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.
|
This looks quite interesting, thanks for working on this! If we go with this change, can you lift the relevant trait into the |
|
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 |
|
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! :) |
|
Closing due to inactivity, but feel free to reopen with a rebase! We've started to grow generics like |
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
cc @alexcrichton
In #13919 we changed
TcpStream::connectto 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::connectandTcpListener::bindto 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:
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_timeouthasn't been modified, and I'm not sure that we want two distinct traits (ToBindSocketAddrandToSocketAddrs) for very similar use-cases. Figure it's at a point where it'll probably drive some discussion, though.