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.

Sign up for GitHub

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

Jump to bottom

Include output stream in panic!() documentation #83254

Merged
merged 4 commits into from
Mar 20, 2021
Merged

Include output stream in panic!() documentation #83254

merged 4 commits into from
Mar 20, 2021

Conversation

jfrimmel
Copy link
Contributor

@jfrimmel jfrimmel commented Mar 18, 2021 •
edited
Loading

Fixes #83252.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Mar 18, 2021
@Dylan-DPC-zz
Copy link

r? @Dylan-DPC

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 18, 2021

📌 Commit 55d9e0f has been approved by Dylan-DPC

@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 18, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Mar 18, 2021 •
edited
Loading

The string built by this macro (using the [`format!`] syntax for building the actual message) is printed to `stderr`.

This is only what the default hook in std does, which can be overridden, or might not even exist (in no_std programs). The panic macro itself doesn't print anything to stderr; the concept of stderr or printing doesn't even exist in core.

I'm not objecting to documenting what happens by default with std, but we shouldn't claim that this behaviour is a property of the panic!() macro itself. Maybe something like "std's default panic hook will ..", with a link to std::panic::set_hook.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 18, 2021
@m-ou-se m-ou-se added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 18, 2021
@jfrimmel
Add more information about panicking
61e5d54 
This includes the description of the default `std` behavior and mentions
the `panic::set_hook()` function.
@jfrimmel
Copy link
Contributor Author

jfrimmel commented Mar 18, 2021 •
edited
Loading

I've incorporated your feedback @m-ou-se. I hope this is both helpful to a reader and concise while still being technically correct. Despite this file being in core, I'd like to mention the std-behavior prominently, as most of the readers come from reading the std docs.

m-ou-se
m-ou-se reviewed Mar 18, 2021
View reviewed changes
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looking better now. I have a few more comments:

library/core/src/macros/panic.md Outdated Show resolved Hide resolved
library/core/src/macros/panic.md Outdated Show resolved Hide resolved
@m-ou-se m-ou-se assigned m-ou-se and unassigned Dylan-DPC-zz Mar 18, 2021
@Dylan-DPC-zz
Copy link

@bors r-

r? @m-ou-se

@jfrimmel
Copy link
Contributor Author

Didn't intend to make this much noise in such a small change, apologies.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 18, 2021

Didn't intend to make this much noise in such a small change, apologies.

Oh I'm very happy you're doing this! Clearly we were missing documentation here. Looks like we didn't even explain anywhere what happens with panic!() in std. So this is an important change you're making!

@m-ou-se
Copy link
Member

m-ou-se commented Mar 18, 2021

This looks great now. Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 18, 2021

📌 Commit d5e45b5 has been approved by m-ou-se

@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 Mar 18, 2021
joshtriplett
joshtriplett requested changes Mar 18, 2021
View reviewed changes
Copy link
Member

@joshtriplett joshtriplett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few typos; otherwise LGTM, and I'm glad to see this documented.

library/core/src/macros/panic.md Outdated Show resolved Hide resolved
library/core/src/macros/panic.md Outdated Show resolved Hide resolved
library/core/src/macros/panic.md Outdated Show resolved Hide resolved
@jfrimmel @joshtriplett
Apply suggestions from code review
19bd066 
Co-authored-by: Josh Triplett <josh@joshtriplett.org>
@joshtriplett
Copy link
Member

Thanks for the quick update! 👍

@m-ou-se
Copy link
Member

m-ou-se commented Mar 19, 2021

@bors r=m-ou-se,joshtriplett

@bors
Copy link
Contributor

bors commented Mar 19, 2021

📌 Commit 19bd066 has been approved by m-ou-se,joshtriplett

@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 19, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2021
@bors
Auto merge of rust-lang#83308 - Dylan-DPC:rollup-p2j6sy8, r=Dylan-DPC
6bfbf0c 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#79986 (Only build help popup when it's really needed)
 - rust-lang#82570 (Add `as_str` method for split whitespace str iterators)
 - rust-lang#83244 (Fix overflowing length in Vec<ZST> to VecDeque)
 - rust-lang#83254 (Include output stream in `panic!()` documentation)
 - rust-lang#83269 (Revert the second deprecation of collections::Bound)
 - rust-lang#83277 (Mark early otherwise optimization unsound)
 - rust-lang#83285 (Update LLVM to bring in SIMD updates for WebAssembly)
 - rust-lang#83297 (Do not ICE on ty::Error as an error must already have been reported)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2cc5d72 into rust-lang:master Mar 20, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 20, 2021
@jfrimmel jfrimmel deleted the panic_output-stream branch October 14, 2021 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-ou-se m-ou-se m-ou-se left review comments

@joshtriplett joshtriplett joshtriplett requested changes

Assignees

@m-ou-se m-ou-se

Labels
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
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.

std::panic macro documentation does not specify where the string is printed to (stdout or stderr)
7 participants
@jfrimmel @rust-highfive @Dylan-DPC-zz @bors @m-ou-se @joshtriplett @rustbot