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 platform-specific differences for std::process::exit. #38397

Merged
merged 1 commit into from
Dec 19, 2016

Conversation

frewsxcv
Copy link
Member

Fixes #35046.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

/// function only considers the lower eight bits of `code`. For example, on
/// these platforms, calling `exit` with values `0x00_00` and `0x0f_00` will be
/// handled identically. Other platforms, like Windows, will take into account
/// all 32 bits of `code`.
Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to add a sentence at the end like:

For specific information about your platform, please check <insert resource here>

But I wasn't sure what resource to put there. man 3 exit? "your systems 'exit' function"?

¯\_(ツ)_/¯

Copy link
Member

Choose a reason for hiding this comment

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

I tend to like to cite the opengroup docs as they've always seemed "extra official". Maybe a link to that could be added 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.

It looks like your opengroup link says exit only takes into consideration the least significant eight bits. I have hesitations about just citing that as it doesn't match the behavior on Windows which takes into account all 32 bits with no truncation (as per this comment by @retep998).

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry to be clear that opengroup page is a Unix-only thing, which is basically just explaining why only 8 bits are transmitted to the parent on Unix.

Copy link
Member

@nagisa nagisa Dec 16, 2016

Choose a reason for hiding this comment

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

Windows is the only tier 1~2 platform which I’m sure passes through all the 32 bits. Every other target may be passing through whatever number of bits they want, as long as its not less than 8 bits. I’m also not sure it is worth mentioning platforms which have obvious behaviour at all (i.e. Windows). Since tiers are an always-moving list, I’d perhaps not tie any explanation to that, but I still feel like it could be worded better. For example:

## Platform-specific behaviour

**Unix**: On UNIX-like (or POSIX, whichever you prefer) platforms it is unlikely that all the 32 bits of 
status code will be visible to a parent process inspecting the exit code. On most UNIX-like platforms 
only 8 least-significant bits are considered/evaluated/whatever is the best word here.

I do not think it is necessary to link to any external references here. I’m confident that mentioning this exceptional behaviour is necessary, but in case of references you’ll either end up having to reference documentation for every single supported platform (some of them have no documentation at all) or linking a POSIX spec, which is useless if you’re trying to figure out exact behaviour on your particular platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great feedback, thanks! I incorporated in your block of text you wrote above and made some minor tweaks.

@alexcrichton
Copy link
Member

Thanks @frewsxcv! In retrospect we should have probably made this argument a u8, but alas.

/// # Examples
///
/// ```
/// use std::process;
///
/// process::exit(0);
/// ```
///
/// Due to [platform-specific behavior], the exit code for this example will be
Copy link
Member

Choose a reason for hiding this comment

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

This is a great example.

@frewsxcv frewsxcv force-pushed the platform-specific-process-exit branch 3 times, most recently from 111ff5c to 0971cb1 Compare December 16, 2016 16:53
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Dec 16, 2016

📌 Commit 0971cb1 has been approved by alexcrichton

@frewsxcv
Copy link
Member Author

@bors rollup

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 18, 2016
…exit, r=alexcrichton

Document platform-specific differences for `std::process::exit`.

Fixes rust-lang#35046.
/// Due to [platform-specific behavior], the exit code for this example will be
/// `0` on Linux, but `256` on Windows:
///
/// ```
Copy link
Member Author

Choose a reason for hiding this comment

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

This is going to fail on Windows, because it's a non-zero return code. So I'm going to mark as no_run.

@frewsxcv frewsxcv force-pushed the platform-specific-process-exit branch from 0971cb1 to 4d392d3 Compare December 18, 2016 17:47
@frewsxcv
Copy link
Member Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Dec 18, 2016

📌 Commit 4d392d3 has been approved by alexcrichton

sanxiyn added a commit to sanxiyn/rust that referenced this pull request Dec 19, 2016
…exit, r=alexcrichton

Document platform-specific differences for `std::process::exit`.

Fixes rust-lang#35046.
bors added a commit that referenced this pull request Dec 19, 2016
Rollup of 9 pull requests

- Successful merges: #38334, #38397, #38413, #38421, #38422, #38433, #38438, #38445, #38459
- Failed merges:
@bors bors merged commit 4d392d3 into rust-lang:master Dec 19, 2016
@frewsxcv frewsxcv deleted the platform-specific-process-exit branch May 1, 2017 21:52
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.

5 participants