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

Documented more clearly why there practically will be no panic. #4

Closed
wants to merge 1 commit into from
Closed

Documented more clearly why there practically will be no panic. #4

wants to merge 1 commit into from

Conversation

rustonaut
Copy link

This also merges to windows+POSIX documenation into one single
doc comment.

This also merges to windows+POSIX documenation into one single
doc comment.
@rustonaut
Copy link
Author

This changes make it more clear why panic is fine.

It also makes sure we have only one doc comment, so that:

  1. We don't have to repeat ourself.
  2. We can see platform specific behavior without needing to build the doc cross platform (or look at the code).

Copy link
Owner

@swsnr swsnr left a comment

Choose a reason for hiding this comment

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

@rustonaut I appreciate your effort but I'm not sure what all this is about… why do you care so much about a situation that can literally never occur?

Personally I don't care that docstrings are separate; it's easy to view docs for other targets on docs.rs, and it feels weird to add an extra function just to merge docstrings. Is the way you've done it the way you normally do this in Rust? Shouldn't the _impl functions be #[inline] then?

As for the text itself please edit it wrt to grammar and spelling.

/// # Panics and why this function doesn't return a error.
///
/// _Theoretically_ this function can fail in a unexpected way and panic on both
/// Windows and POSIX compliant systems as neither of them defines that
Copy link
Owner

Choose a reason for hiding this comment

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

Wrt to Windows that's not true; GetComputerNameExW is pretty clear about what errors can occur.

Copy link
Author

Choose a reason for hiding this comment

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

@lunaryorn I have to apologize for not answering for such a long time (thinks happend ¯_(ツ)_/¯).

t's not that simple. Yes it's clear what errors are returned now. But the whole point about this was that systems might add new error codes or at last theoretically could do so.

This kind of system API is generally designed, so that new error cases can be added any time (through the design originally comes from how syscalls are implemented on x86). Generally speaking if you wrap a syscall you should normally never rely on being able to know about all error cases. Through it's just generally speaking. There are syscalls for which this is less true and after looking into how getting the host name tend to be implemented it belongs into that category I guess. This is also true for windows, just that windows tend to be more hesitant to do any API changes as far as I know, so I guess it doesn't really matter for windows.

Wrt. #[inline] it's a interesting story. In the past I added it to all functions where it seems to make sense. But then I realized that it's better to just let the compiler does it's thing. Part of how I realized it was that rusts libstr removed a lot of #[inline] annotations (or refused to add them). Through then in this specific case it at last wouldn't hurt to add #[inline] to the out function (inlining it into the calling code). But it might hurt to add #[inline] the the inner function as it might confuse the compiler which normally would try to inline the outer function into the calling code, well but then it might also help assuming the best solution is to always inline the whole code into the calling function...

Anyway I won't annoy you anymore. I just wanted to clean up any open questions with this comment.

@swsnr swsnr closed this Aug 20, 2019
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.

None yet

2 participants