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

Change Termination::report return type to ExitCode #93442

Merged
merged 2 commits into from
Feb 1, 2022

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Jan 28, 2022

Related to #43301

The goal of this change is to minimize the forward compatibility risks in stabilizing Termination. By using the opaque type ExitCode instead of an i32 we leave room for us to evolve the API over time to provide what cross-platform consistency we can / minimize footguns when working with exit codes, where as stabilizing on i32 would limit what changes we could make in the future in how we represent and construct exit codes.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 28, 2022
@yaahc yaahc added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 28, 2022
@rust-log-analyzer

This comment has been minimized.

@yaahc
Copy link
Member Author

yaahc commented Jan 28, 2022

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

These test failures are unrelated to the changes in this PR: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Spurious.20mir-opt.20segfaults.20GHA/near/269699225

@yaahc yaahc added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 28, 2022
@Mark-Simulacrum
Copy link
Member

I will note that without even a PartialEq or is_success/is_failure methods, this makes the ExitCode basically useless outside unstable code modulo passing up to fn main. That seems plausibly OK, but fairly weird for an API we would actually want to stabilize.

r=me with the one nit comment fixed and/or perf run to confirm no effects modulo it.

@bors rollup=never since regardless this'll probably perturb codegen for binaries a little.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2022
@yaahc
Copy link
Member Author

yaahc commented Jan 31, 2022

Awesome, resolved the inlining issue. Going to add the partial_eq / is_success question to the ExitCode tracking issue as an unresolved question for now.

@bors r=Mark-Simulacrum rollup=never

@bors
Copy link
Contributor

bors commented Jan 31, 2022

📌 Commit 19db85d has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2022
@bors
Copy link
Contributor

bors commented Feb 1, 2022

⌛ Testing commit 19db85d with merge 2681f25...

@bors
Copy link
Contributor

bors commented Feb 1, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 2681f25 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 1, 2022
@bors bors merged commit 2681f25 into rust-lang:master Feb 1, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 1, 2022
@bors bors mentioned this pull request Feb 1, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2681f25): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 19, 2022
Remove unnecessay .report() on ExitCode

Since rust-lang#93442, the return type is `ExitCode` anyway so there's no need to do a conversion using `.report()` (which is now just a no-op).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants