Clarify ExitStatusExt documentation#158574
Conversation
|
r? @Darksonn rustbot has assigned @Darksonn. Use Why was this reviewer chosen?The reviewer was selected based on:
|
b26864a to
c224bea
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
c224bea to
b77be38
Compare
|
Updated. Replied inline and left comment threads unresolved, assuming you'll do that when you're happy enough with the results. |
| /// hold on every target: | ||
| /// | ||
| /// ``` | ||
| /// # if cfg!(not(target_os = "fuchsia")) { |
There was a problem hiding this comment.
Nit: You can avoid the # } at the end using
| /// # if cfg!(not(target_os = "fuchsia")) { | |
| /// # if cfg!(not(target_os = "fuchsia")) { return; } |
There was a problem hiding this comment.
I like that syntax. It is cleaner. Also, needed to swap the logic. Applied locally and pushed.
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
## Fix struct links There are several structs (`ExitStatus` and `ExitStatusError`) that could be linked, but aren't. Fixed. All links show their struct now instead of previously some showed `process::ExitStatus` when rendered. ## Add `wait` links The docs distinguish `wait` as code, but do not link to **what** wait they're referring to. As this is Unix extension documentation, adding a link to the POSIX standard for `wait`. Showing a construction of an `ExitStatus` for both a signal and an exit code helps to highlight and internalize what **wait status, not an exit status** means. ## Example for `from_raw` Current documentation calls out "The value should be a **wait status, not an exit status**." but it's unclear what exactly that means without a reference or an example. I added a doctest that shows you need to shift by 8 to get the desired result and annotated it with information about how that's derived based on the linked `wait` documentation. ## Example to `ExitStatus::signal` Called out that a signaled process does not also produce an exit code. Added an example. Called out that just because a process reports that it was not terminated via a signal doesn't mean it never got one (it could be handled). Also added a note correlating shell behavior to exit codes: GNU Bash manual: https://web.archive.org/web/20260625050034/https://www.gnu.org/software/bash/manual/bash.html#Exit-Status-1 > [...] a fatal signal whose number is N, Bash uses the value 128+N as the exit status. FreeBSD bash(1) man page: https://man.freebsd.org/bash/1 > The return value [...] 128+n if the command is terminated by signal n. ## Panic scope on `from_raw` This change is based on preference/experience: When I originally read this documentation, it was because I clicked through `Output` -> `ExitStatus`. While the text "Creates a new `ExitStatus` or `ExitStatusError`" is present at the top, I didn't internalize what that meant exactly. Therefore, when I got to the panic section, I was mildly confused. This edit makes the behavior more explicitly attached to the type. While the original format had all of the same information available, it was left for the reader to bridge that "making an ExitStatusError" is referring to `ExitStatus::from_raw` as that's the only constructor (for now). Making this bridge more explicit also future-proofs us for future constructors (if they're ever added) that may or may not panic. While the context of the panic documentation is directly attached to `from_raw`, it's slightly (pedantically) more correct to not imply that ANY way of creating an ExitStatusError` from a `wait` of `0` will panic (a different constructor could perhaps return an option instead). Technically, the syntax isn't correct, as it should be `<ExitStatusError as ExitStatusExt>::from_raw`, but I think this conveys intent without being overly verbose. Co-authored-by: Colin Casey <casey.colin@gmail.com>
b77be38 to
13bcaad
Compare
|
Thanks for the PR! @bors r+ |
Rollup of 8 pull requests Successful merges: - #156737 (Implement `DoubleEndedIterator::next_chunk_back`) - #158180 (std: use `OnceLock` for SGX environment variable storage) - #158427 (Implement `ptr::{read,write}_unaligned` via `repr(packed)`) - #158531 (Change `adjust_ident_and_get_scope` arg to `LocalDefId`) - #158574 (Clarify ExitStatusExt documentation) - #158334 (rustdoc: Show use-site paths for unevaluated const array lengths) - #158364 (rustc_target/asm: add LoongArch LSX/LASX inline asm register support) - #158667 (rustc_sanitizers: use twox-hash without default features)
Rollup merge of #158574 - schneems:schneems/document-exit-status-ext, r=Darksonn Clarify ExitStatusExt documentation ## Fix struct links There are several structs (`ExitStatus` and `ExitStatusError`) that could be linked, but aren't. Fixed. All links show their struct now instead of previously some showed `process::ExitStatus` when rendered. ## Add `wait` links The docs distinguish `wait` as code, but do not link to **what** wait they're referring to. As this is Unix extension documentation, adding a link to the POSIX standard for `wait`. Showing a construction of an `ExitStatus` for both a signal and an exit code helps to highlight and internalize what **wait status, not an exit status** means. ## Example for `from_raw` Current documentation calls out "The value should be a **wait status, not an exit status**." but it's unclear what exactly that means without a reference or an example. I added a doctest that shows you need to shift by 8 to get the desired result and annotated it with information about how that's derived based on the linked `wait` documentation. ## Example to `ExitStatus::signal` Called out that a signaled process does not also produce an exit code. Added an example. Called out that just because a process reports that it was not terminated via a signal doesn't mean it never got one (it could be handled). Also added a note correlating shell behavior to exit codes: GNU Bash manual: https://web.archive.org/web/20260625050034/https://www.gnu.org/software/bash/manual/bash.html#Exit-Status-1 > [...] a fatal signal whose number is N, Bash uses the value 128+N as the exit status. FreeBSD bash(1) man page: https://man.freebsd.org/bash/1 > The return value [...] 128+n if the command is terminated by signal n. ## Panic scope on `from_raw` This change is based on preference/experience: When I originally read this documentation, it was because I clicked through `Output` -> `ExitStatus`. While the text "Creates a new `ExitStatus` or `ExitStatusError`" is present at the top, I didn't internalize what that meant exactly. Therefore, when I got to the panic section, I was mildly confused. This edit makes the behavior more explicitly attached to the type. While the original format had all of the same information available, it was left for the reader to bridge that "making an ExitStatusError" is referring to `ExitStatus::from_raw` as that's the only constructor (for now). Making this bridge more explicit also future-proofs us for future constructors (if they're ever added) that may or may not panic. While the context of the panic documentation is directly attached to `from_raw`, it's slightly (pedantically) more correct to not imply that ANY way of creating an ExitStatusError` from a `wait` of `0` will panic (a different constructor could perhaps return an option instead). Technically, the syntax isn't correct, as it should be `<ExitStatusError as ExitStatusExt>::from_raw`, but I think this conveys intent without being overly verbose.
|
Thanks for the review and all your maintenance work! These are somewhat niche interfaces, but it feels good that anyone who needs them doesn't have to re-learn all this from scratch. Now I'm feeling like POSIX is missing an interface for constructing these exit values. I've never contributed there, but seeing no similar discussion, it might be worth opening one. |
|
I don't know anything about contributing to POSIX unfortunately. |
View all comments
Fix struct links
There are several structs (
ExitStatusandExitStatusError) that could be linked, but aren't. Fixed. All links show their struct now instead of previously some showedprocess::ExitStatuswhen rendered.Add
waitlinksThe docs distinguish
waitas code, but do not link to what wait they're referring to. As this is Unix extension documentation, adding a link to the POSIX standard forwait. Showing a construction of anExitStatusfor both a signal and an exit code helps to highlight and internalize what wait status, not an exit status means.Example for
from_rawCurrent documentation calls out "The value should be a wait status, not an exit status." but it's unclear what exactly that means without a reference or an example. I added a doctest that shows you need to shift by 8 to get the desired result and annotated it with information about how that's derived based on the linked
waitdocumentation.Example to
ExitStatus::signalCalled out that a signaled process does not also produce an exit code. Added an example. Called out that just because a process reports that it was not terminated via a signal doesn't mean it never got one (it could be handled). Also added a note correlating shell behavior to exit codes:
GNU Bash manual: https://web.archive.org/web/20260625050034/https://www.gnu.org/software/bash/manual/bash.html#Exit-Status-1
FreeBSD bash(1) man page: https://man.freebsd.org/bash/1
Panic scope on
from_rawThis change is based on preference/experience:
When I originally read this documentation, it was because I clicked through
Output->ExitStatus. While the text "Creates a newExitStatusorExitStatusError" is present at the top, I didn't internalize what that meant exactly. Therefore, when I got to the panic section, I was mildly confused.This edit makes the behavior more explicitly attached to the type. While the original format had all of the same information available, it was left for the reader to bridge that "making an ExitStatusError" is referring to
ExitStatus::from_rawas that's the only constructor (for now).Making this bridge more explicit also future-proofs us for future constructors (if they're ever added) that may or may not panic. While the context of the panic documentation is directly attached to
from_raw, it's slightly (pedantically) more correct to not imply that ANY way of creating an ExitStatusErrorfrom awaitof0` will panic (a different constructor could perhaps return an option instead).Technically, the syntax isn't correct, as it should be
<ExitStatusError as ExitStatusExt>::from_raw, but I think this conveys intent without being overly verbose.