Add set_status for std::process::Child
#69843
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (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. |
rust-highfive
added
the
S-waiting-on-review
|
Reassigning to another libs member. I don't think @withoutboats has been reviewing recently. r? @sfackler |
src/libstd/process.rs
Outdated
| /// [`wait`]: #method.wait | ||
| /// [`wait_with_output`]: #method.wait_with_output | ||
| #[stable(feature = "child_set_status", since = "1.44.0")] | ||
| pub unsafe fn set_status(&mut self, status: ExitStatus) { |
There was a problem hiding this comment.
In what way can calling this method incorrectly cause memory unsafety?
There was a problem hiding this comment.
My original reasoning was that kill would be unexpectedly affected, and there could be other internal uses by Child in the future. However, thinking through it again, it does make sense to remove the unsafe.
|
I'm somewhat worried that this opens too many boxes about the internal representation of Child - thoughts @rust-lang/libs? |
|
If there's a warning that setting it to an inaccurate exit status results in undefined behavior, there shouldn't be a problem. It would require unsafe code to get the exit status externally anyway. However, after updating the wait implementation in my crate, I actually no longer need to call this function. So, I am not entirely opposed to delaying it if there are objections. |
|
Ah cool yeah, if your code isn't going to be depending on the functionality, then I think it makes sense to close this for now then. |
|
That works. Thanks for the review anyway! |
Closes #39188
Are tests other than the doctest needed? Any good test would need to be very platform-specific.
I also removed
ExitStatus::new, since it was only used for some platforms and could be replaced with constructing the struct directly.