-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Better docs and associated SUCCESS/FAILURE for process::ExitCode #48618
Conversation
src/libstd/process.rs
Outdated
/// the [`SUCCESS`] and [`FAILURE`] associated items. | ||
/// | ||
/// [`SUCCESS`]: #constant.SUCCESS | ||
/// [`FAILURE`]: #constant.FAILURE |
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.
[01:29:50] std/process/struct.ExitCode.html:68: broken link fragment `#constant.SUCCESS` pointing to `std/process/struct.ExitCode.html`
[01:29:50] std/process/struct.ExitCode.html:68: broken link fragment `#constant.FAILURE` pointing to `std/process/struct.ExitCode.html`
[01:29:56] thread 'main' panicked at 'found some broken links', tools/linkchecker/main.rs:49:9
src/libstd/process.rs
Outdated
#[cfg(target_arch = "wasm32")] | ||
mod rawexit { | ||
pub const SUCCESS: i32 = 0; | ||
pub const FAILURE: i32 = 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.
do we still need this?
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.
Er I'm not sure I added this... This actually should be rejected by the tidy lint as well...
In any case seems fine to have if it's needed, we can clean it up later
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.
This private mod is originally from https://github.com/rust-lang/rust/pull/46479/files#diff-7dbed9a9f4aeab33f13e792758ac1ed5R13
Did you mean not have the wasm check? Or not have the mod? I can do either or both...
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.
Er sorry what I mean is that we have lint which forbids #[cfg]
in most of the standard library in an attempt to centralize all platform-specific code to src/libstd/sys
rather than having it sprinkled throughout the standard library itself.
It's ok to not fix this up here, but if you're feeling enterprising this would just involve moving the modules to src/libstd/sys
and using constants from there rather than this location.
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 main question I had was "does wasm offer the standard constants" -- note though that this code is pre-existing, though it's been moved around, so the lint must not work that well...
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.
r=me, but i'd like to know whether we still need the "wasm exception"
/// | ||
/// **Warning**: While various forms of this were discussed in [RFC #1937], | ||
/// it was ultimately cut from that RFC, and thus this type is more subject | ||
/// to change even than the usual unstable item churn. |
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.
as an aside, I just ran into a place where I really wanted this today. I was already calling exit(1)
-- so it seems silly not to provide a way to "return" that...
@scottmcm do you want to make the changes @alexcrichton suggested? |
Now begins the saga of fixing compilation errors on other platforms...
61cc4f0
to
74c5c6e
Compare
8610f07
to
74c5c6e
Compare
Ok, hopefully this is good now. Made a new tracking issue for the type, #48711 Travis job to see if I typo'd anything in wasm: https://travis-ci.org/rust-lang/rust/builds/348817793 |
I can't really comment on the |
@bors: r+ |
📌 Commit 74c5c6e has been approved by |
…crichton Better docs and associated SUCCESS/FAILURE for process::ExitCode Follow-up to rust-lang#48497 (comment), since that PR was the minimal thing to unblock rust-lang#48453 (comment). r? @nikomatsakis
…atrieb `pal::unsupported::process::ExitCode`: use an `u8` instead of a `bool` `ExitCode` should “represents the status code the current process can return to its parent under normal termination”, but is currently represented as a `bool` on unsupported platforms, making the `impl From<u8> for ExitCode` lossy. Fixes rust-lang#130532. History: [IRLO thread](https://internals.rust-lang.org/t/mini-pre-rfc-redesigning-process-exitstatus/5426) (`ExitCode` as a `main` return), rust-lang#48618 (initial impl), rust-lang#93445 (`From<u8>` impl).
Rollup merge of rust-lang#130554 - ShE3py:unsupported-exitcode, r=Noratrieb `pal::unsupported::process::ExitCode`: use an `u8` instead of a `bool` `ExitCode` should “represents the status code the current process can return to its parent under normal termination”, but is currently represented as a `bool` on unsupported platforms, making the `impl From<u8> for ExitCode` lossy. Fixes rust-lang#130532. History: [IRLO thread](https://internals.rust-lang.org/t/mini-pre-rfc-redesigning-process-exitstatus/5426) (`ExitCode` as a `main` return), rust-lang#48618 (initial impl), rust-lang#93445 (`From<u8>` impl).
Follow-up to #48497 (comment), since that PR was the minimal thing to unblock #48453 (comment).
r? @nikomatsakis