-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Expand documentation of process::exit and exec #38518
Conversation
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
@bors: delegate=kmcallister r? @kmcallister |
✌️ @keeganmacallister can now approve this pull request |
✌️ @kmcallister can now approve this pull request |
/// 1 | ||
/// } | ||
/// }); | ||
/// } | ||
/// ``` |
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.
What do you think about this as an example:
use std::io::{self, Write};
use std::process;
fn run_app() -> Result<(), ()> {
// Your application logic here
Ok(())
}
fn main() {
process::exit(match run_app() {
Ok(_) => 0,
Err(_) => 1,
});
}
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.
I personally prefer to not include an import for only one use of a function. I incorporated some of the other changes.
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.
Why are you printing the error if the error type is ()
?
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.
I suppose I can see why, but it does seem unrelated to the point of this example. Not a big deal.
@@ -67,10 +67,20 @@ pub trait CommandExt { | |||
/// an error indicating why the exec (or another part of the setup of the | |||
/// `Command`) failed. | |||
/// | |||
/// `exec` not returning has the same implications as calling | |||
/// [`process::exit`] – no destructors on the current stack or any other | |||
/// thread’s stack will be run. Therefore, it is recommended to only call |
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.
Would it be more accurate to replace "any other thread's" with "child thread's"?
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.
No. “Child thread” makes sense in relation to “main thread”, but you can call process::exit on any thread you want.
/// `exec` at a point where it is fine to not run any destructors. Note, | ||
/// that `execvp` syscall independently guarantees that all memory is freed | ||
/// and all file descriptors with the `CLOEXEC` option (set by default on all | ||
/// file descriptors opened by the standard library) are closed. |
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.
Is this platform specific behavior? I'm not familiar with how all the platforms handle execvp
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.
It is not. We always try our best to set CLOEXEC on all filedescriptors in the standard library and the behaviour of execvp
wrt CLOEXEC
fds is documented in posix: http://pubs.opengroup.org/onlinepubs/9699919799/functions/execvp.html
/// [`process::exit`] – no destructors on the current stack or any other | ||
/// thread’s stack will be run. Therefore, it is recommended to only call | ||
/// `exec` at a point where it is fine to not run any destructors. Note, | ||
/// that `execvp` syscall independently guarantees that all memory is freed |
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.
Nit: that the execvp
syscall
@@ -875,10 +875,23 @@ impl Child { | |||
/// | |||
/// # Examples | |||
/// | |||
/// Due to this function’s behaviour regarding destructors, a conventional way | |||
/// to use the function is to extract the actual computation to another | |||
/// function and compute exit code from its return value: |
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.
Nit: compute the exit code
Ping? |
@@ -875,10 +875,27 @@ impl Child { | |||
/// | |||
/// # Examples | |||
/// | |||
/// Due to this function’s behaviour regarding destructors, a conventional way |
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.
I think the documentation uses American English, so this should be "behavior"
Show a conventional way to use process::exit when destructors are considered important and also mention that the same caveats wrt destructors apply to exec as well.
@bors: r+ |
📌 Commit c2eab73 has been approved by |
@bors rollup |
Expand documentation of process::exit and exec Show a conventional way to use process::exit when destructors are considered important and also mention that the same caveats wrt destructors apply to exec as well.
Expand documentation of process::exit and exec Show a conventional way to use process::exit when destructors are considered important and also mention that the same caveats wrt destructors apply to exec as well.
Show a conventional way to use process::exit when destructors are considered important and also
mention that the same caveats wrt destructors apply to exec as well.