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

Document that UdpSocket::recv and recv_from do not read from the buff… #740

Closed
wants to merge 1 commit into from

Conversation

frehberg
Copy link
Contributor

@frehberg frehberg commented Oct 3, 2017

…er (#657)

Copy link

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @frehberg! I've just left a few small comments that apply to both doc additions.

src/udp.rs Outdated
/// excess bytes may be discarded.
///
/// The function is overwriting previous content of `buf`. Slicing
/// `&buf[..NumRead]` allows efficient access and boundary checking at compile time.
Copy link

Choose a reason for hiding this comment

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

Assuming NumRead is the result of recv_from I think a more idiomatic name for it is num_read, so we know it's some local variable.

src/udp.rs Outdated
@@ -82,6 +82,13 @@ impl UdpSocket {

/// Receives data from the socket. On success, returns the number of bytes
/// read and the address from whence the data came.
///
/// The function must be called with valid byte array `buf` of sufficient size to
Copy link

Choose a reason for hiding this comment

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

I think we still need to mention that this function only writes into the buffer and never reads from it.

src/udp.rs Outdated
/// the number of bytes read and the address from whence the data came.
/// the number of bytes read.
///
/// The function must be called with valid byte array `buf` of sufficient size to
Copy link

Choose a reason for hiding this comment

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

I think we still need to mention that this function only writes into the buffer and never reads from it.

@frehberg
Copy link
Contributor Author

frehberg commented Oct 4, 2017

API-Doc has been extended, adding these aspects. I had to re-arrange the text a bit, does it please?

@carllerche
Copy link
Member

Thanks... this looks good to me. Unfortunately, it looks like Android CI broke... which going to block me from merging until it is fixed 😢

@carllerche
Copy link
Member

A CI fix has been merged to master. Would you mind rebasing when you have a moment? Thanks!

@KodrAus
Copy link

KodrAus commented Nov 8, 2017

@frehberg How's it going? Would you like a hand rebasing this?

@carllerche
Copy link
Member

Something wonky happened with this PR. The commit was rebased, but didn't make it to the PR. I moved it here: #775

@carllerche carllerche closed this Dec 15, 2017
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