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

std: <ExitStatus as Display>::fmt name the signal it died from #95833

Merged
merged 3 commits into from
Jun 3, 2022

Conversation

notriddle
Copy link
Contributor

Related to #95601

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Apr 9, 2022
@clarfonthey
Copy link
Contributor

FWIW I actually have written code to do this myself and think it might be a lot more comfortable to directly use the constants from libc rather than to hard-code numbers in the match statements, since it will be easier to detect when signal numbers differ across targets. Since I assume we can't just import the libc crate for libstd, we would likely just inline the code that defines the constants here.

For example, defining a SIGINT constant whose value depends on target and then adding SIGINT as the LHS of the match statement. If certain signals are only available on certain targets, match arms can just be #[cfg]ed for those targets.

@notriddle
Copy link
Contributor Author

I would love to use the functions from libc, but all the ones I could find were thread-unsafe.

@clarfonthey
Copy link
Contributor

To clarify, I don't mean using the functions from libc, but rather using the definitions of the constants for each of the signals, like libc::SIGINT. Right now the constants are hard-coded into the match arms and each target has its own match expression, which means that it's harder to see, for example, what the differences between the various platforms are, and what's supported where.

@notriddle notriddle force-pushed the notriddle/human-readable-signals branch from 935050d to e985ed0 Compare April 23, 2022 16:26
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 23, 2022
@notriddle
Copy link
Contributor Author

You're right. I didn't even think of that. That's much simpler.

@notriddle notriddle force-pushed the notriddle/human-readable-signals branch from e985ed0 to 47030d3 Compare April 23, 2022 18:54
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2022
@GuillaumeGomez
Copy link
Member

Let's ping the libs team for the approval.

cc @rust-lang/libs

@yaahc yaahc assigned yaahc and unassigned kennytm Jun 1, 2022
@yaahc
Copy link
Member

yaahc commented Jun 2, 2022

Looks great, ty!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2022

📌 Commit 267a6c8 has been approved by yaahc

@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 Jun 2, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 2, 2022
…signals, r=yaahc

std: `<ExitStatus as Display>::fmt` name the signal it died from

Related to rust-lang#95601
@matthiaskrgr
Copy link
Member

@bors r- failed in a rollup: #97662 (comment)

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 2, 2022
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 2, 2022
@notriddle notriddle force-pushed the notriddle/human-readable-signals branch from c8d8e20 to b767887 Compare June 2, 2022 22:19
@notriddle notriddle force-pushed the notriddle/human-readable-signals branch from b767887 to 22791bb Compare June 2, 2022 22:28
@notriddle
Copy link
Contributor Author

Sorry, @yaahc. I've added another commit to address the MIPS trouble.

@yaahc
Copy link
Member

yaahc commented Jun 3, 2022

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 3, 2022

📌 Commit 22791bb has been approved by yaahc

@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 Jun 3, 2022
@bors
Copy link
Contributor

bors commented Jun 3, 2022

⌛ Testing commit 22791bb with merge a6b8c69...

@bors
Copy link
Contributor

bors commented Jun 3, 2022

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing a6b8c69 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2022
@bors bors merged commit a6b8c69 into rust-lang:master Jun 3, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 3, 2022
@notriddle notriddle deleted the notriddle/human-readable-signals branch June 3, 2022 22:43
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a6b8c69): comparison url.

Instruction count

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
0.3% 0.3% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
0.1% 0.1% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.5% -2.5% 1
All 😿🎉 (primary) 0.1% 0.1% 1

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
2.3% 2.3% 1
Improvements 🎉
(primary)
-2.0% -2.2% 2
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -2.0% -2.2% 2

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

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

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 Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.