-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
exit: document interaction with C #136320
Conversation
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 it would be nice to acknowledge the movement in POSIX and the C standard to define this as thread-safe in the future. We also do the same for setenv
where we link to discussions about its unsafety.
library/std/src/process.rs
Outdated
@@ -2294,6 +2294,25 @@ impl Child { | |||
/// considered undesirable. Note that returning from `main` also calls `exit`, so making `exit` an | |||
/// unsafe operation is not an option.) | |||
/// | |||
/// ## Safe interop with C code |
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.
/// ## Safe interop with C code | |
/// ## Safety |
I think usually, this section is called "Safety".
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 don't think we have any safe function with a "safety" section.
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.
We used to have one for the std::env::{set_var, remove_var}
functions, even before they were marked as unsafe
. I think this is a similar situation.
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.
We decided the situation is not quite similar, since we won't make exit
unsafe.
048b18c
to
8f3adf5
Compare
This text is absolutely unconvincing ( https://doc.rust-lang.org/nightly/std/process/fn.exit.html ):
Rust's thread API (i. e. So, returning from I'm not saying that we should mark |
First of all, that text is pre-existing, not touched by this PR. So it's kind of off-topic in this PR. You should at least have made that clear in your comment; currently you are implying that you are commenting on something I wrote. (Maybe that's what you meant by adding the URL? It's not clear.) Second, I don't follow your argument. Note that that text talks about the requirements imposed on
The former would imply that I don't know what you are talking about; the fact that one can return from |
@RalfJung
Yes, this is what I meant. I thought this is good idea to fix whole Okay, now I see you are right |
r? libs-api |
@the8472 maybe you could take a look at this one? |
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 notice that nowhere it's explicitly said that we're calling C's exit
here and not _exit
, quick_exit
or whatever.
The added paragraph is only relevant if we're actually calling a function which has these issues. So either we commit to calling this function (which woul constitute a new API guarantee) or add the caveat "on platforms where this is implemented as C's exit()".
@bors r+ rollup |
exit: document interaction with C Cc rust-lang#126600
exit: document interaction with C Cc rust-lang#126600
Rollup of 7 pull requests Successful merges: - rust-lang#136320 (exit: document interaction with C) - rust-lang#138301 (Implement `read_buf` for Hermit) - rust-lang#138508 (Clarify "owned data" in E0515.md) - rust-lang#138556 (Fix ICE: attempted to remap an already remapped filename) - rust-lang#138569 (rustdoc-json: Add tests for `#[repr(...)]`) - rust-lang#138608 (rustc_target: Add target feature constraints for LoongArch) - rust-lang#138619 (Flatten `if`s in `rustc_codegen_ssa`) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#136320 (exit: document interaction with C) - rust-lang#138080 (Leave a breadcrumb towards bootstrap config documentation in `bootstrap.toml`) - rust-lang#138301 (Implement `read_buf` for Hermit) - rust-lang#138569 (rustdoc-json: Add tests for `#[repr(...)]`) - rust-lang#138635 (Extract `for_each_immediate_subpat` from THIR pattern visitors) - rust-lang#138642 (Unvacation myself) - rust-lang#138644 (Add `#[cfg(test)]` for Transition in dfa in `rustc_transmute`) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#136320 (exit: document interaction with C) - rust-lang#138080 (Leave a breadcrumb towards bootstrap config documentation in `bootstrap.toml`) - rust-lang#138301 (Implement `read_buf` for Hermit) - rust-lang#138569 (rustdoc-json: Add tests for `#[repr(...)]`) - rust-lang#138635 (Extract `for_each_immediate_subpat` from THIR pattern visitors) - rust-lang#138642 (Unvacation myself) - rust-lang#138644 (Add `#[cfg(test)]` for Transition in dfa in `rustc_transmute`) r? `@ghost` `@rustbot` modify labels: rollup
exit: document interaction with C Cc rust-lang#126600
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#136320 (exit: document interaction with C) - rust-lang#138080 (Leave a breadcrumb towards bootstrap config documentation in `bootstrap.toml`) - rust-lang#138301 (Implement `read_buf` for Hermit) - rust-lang#138569 (rustdoc-json: Add tests for `#[repr(...)]`) - rust-lang#138635 (Extract `for_each_immediate_subpat` from THIR pattern visitors) - rust-lang#138642 (Unvacation myself) - rust-lang#138644 (Add `#[cfg(test)]` for Transition in dfa in `rustc_transmute`) r? `@ghost` `@rustbot` modify labels: rollup
Cc #126600