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 more examples to UpdSocket #38067

Merged
merged 1 commit into from
Dec 13, 2016
Merged

Conversation

GuillaumeGomez
Copy link
Member

/// use std::net::UdpSocket;
///
/// let socket = UdpSocket::bind("127.0.0.1:34254").expect("couldn't bind to address");
/// socket.take_error().expect("no error was expected");
Copy link
Member

Choose a reason for hiding this comment

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

Can you just assert for either Ok(None) or Ok(Some(something)) or Err(something)? This is very mind-bending.

Copy link
Member

Choose a reason for hiding this comment

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

I think I might like this example more if it was something like:

let error = socket.take_error.expect("could not take error");
if let Some(error) = error {
    println!("UDP socket error: {:?}", error);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@frewsxcv: Too biased. It looks like you want the error instead of checking if you have one. I don't like doing this way. (I hope you got what I wanted to say haha.)

@nagisa: I can try to simplify.

/// use std::net::UdpSocket;
///
/// let socket = UdpSocket::bind("127.0.0.1:34254").expect("couldn't bind to address");
/// socket.connect("127.0.0.1:8080").expect("couldn't connect to server");
Copy link
Member

Choose a reason for hiding this comment

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

I would omit “to server” since UDP is a connection-less protocol and all this does is set the default addresses to send to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, you connect to something so it'd be strange to not precise the server part. :-/

Copy link
Member

@nagisa nagisa Nov 30, 2016

Choose a reason for hiding this comment

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

No, you do not connect to anything with UDP. Server will never know that you… uh… “connect” to it. The only thing that really changes is some internal field called default_send_address on the machine which… uh… “connects”.

connect method on UdpSocket is only named this way because its named this way on the UNIX side, but you oughtn’t be misled by the naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I see your point.

@frewsxcv frewsxcv removed their assignment Dec 4, 2016
@GuillaumeGomez
Copy link
Member Author

Updated.

/// let socket = UdpSocket::bind("127.0.0.1:34254").expect("couldn't bind to address");
/// socket.connect("127.0.0.1:8080").expect("connect function failed");
/// let mut buf = [0; 10];
/// match socket.recv(&mut buf) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You're using expect everywhere else, why not here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, I found this way to construct code better.

@frewsxcv
Copy link
Member

frewsxcv commented Dec 4, 2016

LGTM. @nagisa Have any other comments?

Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

No more comments.

@frewsxcv
Copy link
Member

frewsxcv commented Dec 4, 2016

@GuillaumeGomez r=frewsxcv,nagisa unless you want to address my nit above

@GuillaumeGomez
Copy link
Member Author

@bors r=frewsxcv,nagisa

@bors
Copy link
Contributor

bors commented Dec 9, 2016

📌 Commit 8b0b2b6 has been approved by frewsxcv,nagisa

@frewsxcv
Copy link
Member

frewsxcv commented Dec 9, 2016

@bors rollup

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Dec 12, 2016
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Dec 12, 2016
bors added a commit that referenced this pull request Dec 13, 2016
Rollup of 7 pull requests

- Successful merges: #37052, #37941, #38067, #38164, #38202, #38264, #38299
- Failed merges:
@bors bors merged commit 8b0b2b6 into rust-lang:master Dec 13, 2016
@GuillaumeGomez GuillaumeGomez deleted the udp-doc branch December 13, 2016 11:17
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.

4 participants