-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Hide unnecessary error checking from the user #22739
Conversation
This affects the `set_non_blocking` function which cannot fail for Unix or Windows, given correct parameters. Additionally, the short UDP write error case has been removed as there is no such thing as "short UDP writes", instead, the operating system will error out if the application tries to send a packet larger than the MTU of the network path.
r? @pcwalton (rust_highfive has picked a reviewer for you, use r? to override) |
What's the motivation for doing this? It seems backwards to explicitly remove this error handling. |
The motivation for this is that the only errors There is also no such thing as a partially sent UDP packet, so if the OS returns that it has been partially sent, something has already gone horribly wrong (see also the commit message). |
Err(last_error()) | ||
} else { | ||
Ok(()) | ||
} | ||
}).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this panic in the true-path and leave out the else path instead of unwrapping a fresh Result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be equivalent and will provide the contents of a potential error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropping the else-path and calling unwrap()
on Err(last_error())
would increase readability imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
This affects the `set_non_blocking` function which cannot fail for Unix or Windows, given correct parameters. Additionally, the short UDP write error case has been removed as there is no such thing as \"short UDP writes\", instead, the operating system will error out if the application tries to send a packet larger than the MTU of the network path.
Needs Manishearth@e61a790 ; Manishearth@2470fa1 to compile on windows. |
⌛ Testing commit 0fc1a7d with merge 2baaabd... |
💔 Test failed - auto-win-64-opt |
This affects the
set_non_blocking
function which cannot fail for Unix orWindows, given correct parameters. Additionally, the short UDP write error case
has been removed as there is no such thing as "short UDP writes", instead, the
operating system will error out if the application tries to send a packet
larger than the MTU of the network path.