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

Hide unnecessary error checking from the user #22739

Merged
merged 2 commits into from
Feb 25, 2015
Merged

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Feb 23, 2015

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.

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.
@rust-highfive
Copy link
Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

What's the motivation for doing this? It seems backwards to explicitly remove this error handling.

@tbu-
Copy link
Contributor Author

tbu- commented Feb 24, 2015

The motivation for this is that the only errors ioctl or ioctlsocket can return are related to misuse of the API (passing in pointer to invalid memory, not passing in a correct fd, ...), and bubbling those up to the user is not useful.

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();
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@alexcrichton
Copy link
Member

@bors: r+ 0fc1a7d

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 24, 2015
 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.
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 25, 2015
@Manishearth
Copy link
Member

Needs Manishearth@e61a790 ; Manishearth@2470fa1 to compile on windows.

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 25, 2015
@bors
Copy link
Contributor

bors commented Feb 25, 2015

⌛ Testing commit 0fc1a7d with merge 2baaabd...

@bors
Copy link
Contributor

bors commented Feb 25, 2015

💔 Test failed - auto-win-64-opt

@bors bors merged commit 0fc1a7d into rust-lang:master Feb 25, 2015
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.

7 participants