Skip to content
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

Merged
merged 3 commits into from
Mar 8, 2018

Conversation

scottmcm
Copy link
Member

Follow-up to #48497 (comment), since that PR was the minimal thing to unblock #48453 (comment).

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2018
/// the [`SUCCESS`] and [`FAILURE`] associated items.
///
/// [`SUCCESS`]: #constant.SUCCESS
/// [`FAILURE`]: #constant.FAILURE
Copy link
Member Author

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

#[cfg(target_arch = "wasm32")]
mod rawexit {
pub const SUCCESS: i32 = 0;
pub const FAILURE: i32 = 1;
Copy link
Contributor

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?

cc @alexcrichton

Copy link
Member

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

Copy link
Member Author

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...

Copy link
Member

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.

Copy link
Contributor

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...

Copy link
Contributor

@nikomatsakis nikomatsakis left a 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.
Copy link
Contributor

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...

@nikomatsakis
Copy link
Contributor

@scottmcm do you want to make the changes @alexcrichton suggested?

Now begins the saga of fixing compilation errors on other platforms...
@scottmcm scottmcm force-pushed the elaborate-exitcode branch from 61cc4f0 to 74c5c6e Compare March 4, 2018 02:45
@scottmcm scottmcm changed the title Better docs and associated SUCCESS/FAILURE for process::ExitCode [DO NOT MERGE] Better docs and associated SUCCESS/FAILURE for process::ExitCode Mar 4, 2018
@scottmcm scottmcm force-pushed the elaborate-exitcode branch from 8610f07 to 74c5c6e Compare March 4, 2018 06:53
@scottmcm scottmcm changed the title [DO NOT MERGE] Better docs and associated SUCCESS/FAILURE for process::ExitCode Better docs and associated SUCCESS/FAILURE for process::ExitCode Mar 4, 2018
@scottmcm
Copy link
Member Author

scottmcm commented Mar 4, 2018

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

@nikomatsakis
Copy link
Contributor

I can't really comment on the imp module business.

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 5, 2018

📌 Commit 74c5c6e has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 7, 2018
…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
bors added a commit that referenced this pull request Mar 7, 2018
Rollup of 9 pull requests

- Successful merges: #48511, #48549, #48618, #48624, #48651, #48698, #48778, #48787, #48802
- Failed merges: #48669, #48710
bors added a commit that referenced this pull request Mar 7, 2018
Rollup of 9 pull requests

- Successful merges: #48511, #48549, #48618, #48624, #48651, #48698, #48778, #48787, #48802
- Failed merges: #48669, #48710
bors added a commit that referenced this pull request Mar 7, 2018
Rollup of 9 pull requests

- Successful merges: #48511, #48549, #48618, #48624, #48651, #48698, #48778, #48787, #48802
- Failed merges: #48669, #48710
@alexcrichton alexcrichton merged commit 74c5c6e into rust-lang:master Mar 8, 2018
@scottmcm scottmcm deleted the elaborate-exitcode branch March 8, 2018 01:54
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 19, 2024
…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).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2024
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants