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

Add setter methods to SocketAddr{,V4,V6} #31083

Merged
merged 3 commits into from
Feb 11, 2016
Merged

Conversation

SimonSapin
Copy link
Contributor

As demonstrated in the resolve_socket_addr change, this is less awkward than re-creating a new address from the other parts.

If this is to be accepted, pleas open a tracking issue (I can’t set the appropriate tags) and I’ll update the PR with the tracking issue number.

@rust-highfive
Copy link
Collaborator

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.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

We probably want to add set_foo accessors for all components rather than just port (e.g. ip, flowinfo, ...), but otherwise seems fine to me.

@frewsxcv
Copy link
Member

/home/travis/build/rust-lang/rust/src/libstd/net/addr.rs:86: line longer than 100 chars
/home/travis/build/rust-lang/rust/src/libstd/net/addr.rs:122: line longer than 100 chars
/home/travis/build/rust-lang/rust/src/libstd/net/addr.rs:157: line longer than 100 chars

@SimonSapin
Copy link
Contributor Author

All 3 of these lines are too long because of // FIXME add tracking issue which will be removed before this lands.

@alexcrichton Do you want all setters in this PR, or is that something for later?

@alexcrichton
Copy link
Member

I'd personally prefer to have all the setters in this PR, but oops I forgot to tag this with T-libs to ensure it came up during triage.

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

The libs team discussed this during triage today and the conclusion was that this is functionality that seems good to have to us.

@SimonSapin could you update with setters for the other fields (e.g. ip, flowinfo, etc) and also with a tracking issue?

@alexcrichton alexcrichton removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 4, 2016
As demonstrated in the `resolve_socket_addr` change, this is less awkward
than re-creating a new address from the other parts.
@SimonSapin SimonSapin changed the title Add SocketAddr{,V4,V6}::set_port. Add setter methods to SocketAddr{,V4,V6} Feb 11, 2016
@SimonSapin
Copy link
Contributor Author

@alexcrichton done.

The only thing of note is that when SocketAddr::set_ip is called with a v4 self and a v6 new IP, flowinfo and scope_id default to zero like in SocketAddr::new. The other way around they are dropped. All in all, SocketAddr::set_ip only keeps the port number from self when the version changes.

@SimonSapin
Copy link
Contributor Author

I thought this was not worth mentioning in the doc-comment since SocketAddr::new doesn’t either.

@alexcrichton
Copy link
Member

@bors: r+ 3de820e

Thanks @SimonSapin! These are exactly the semantics I was expecting to have :)

@bors
Copy link
Contributor

bors commented Feb 11, 2016

⌛ Testing commit 3de820e with merge 7342dd8...

bors added a commit that referenced this pull request Feb 11, 2016
As demonstrated in the `resolve_socket_addr` change, this is less awkward than re-creating a new address from the other parts.

If this is to be accepted, pleas open a tracking issue (I can’t set the appropriate tags) and I’ll update the PR with the tracking issue number.
@bors bors merged commit 3de820e into rust-lang:master Feb 11, 2016
@SimonSapin SimonSapin deleted the set_port branch February 12, 2016 08:50
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.

6 participants