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

document #[panic_handler] #75

Merged
merged 4 commits into from
Sep 7, 2018
Merged

document #[panic_handler] #75

merged 4 commits into from
Sep 7, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Jun 20, 2018

@Mark-Simulacrum
Copy link
Member

@steveklabnik Can we get a review here today? I'd like to merge this quickly so we can have docs ready to go for Edition RC.

@japaric
Copy link
Member Author

japaric commented Sep 7, 2018

This needs to be updated because it's still referring to panic_implementation, the old name of panic_handler. But I gotta run right now so I'll get back to this in a few hours.

@steveklabnik
Copy link
Member

steveklabnik commented Sep 7, 2018 via email

@japaric
Copy link
Member Author

japaric commented Sep 7, 2018

@steveklabnik updated!

kennytm added a commit to kennytm/rust that referenced this pull request Sep 7, 2018
…imulacrum

stabilize #[panic_handler]

closes rust-lang#44489

### Update(2018-09-07)

This was proposed for stabilization in rust-lang#44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in rust-lang#44489 (comment)

Documentation PRs:

- Reference. rust-lang/reference#362
- Nomicon. rust-lang/nomicon#75

---

`#[panic_implementation]` was implemented recently in rust-lang#50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. rust-lang#44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in rust-lang#43054

Some unresolved questions from rust-lang#44489:

> Should the Display of PanicInfo format the panic information as "panicked at 'reason',
> src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

> Is this design compatible, or can it be extended to work, with unwinding implementations for
> no-std environments?

I believe @whitequark made more progress with unwinding in no-std since their last comment in rust-lang#44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp
Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

One more last nit; sorry I didn't see this before!

-> !` and such function must appear *once* in the dependency graph of a binary / dylib / cdylib
crate. The API of `PanicInfo` can be found in the [API docs].

[API docs]: https://doc.rust-lang.org/nightly/core/panic/struct.PanicInfo.html
Copy link
Member

Choose a reason for hiding this comment

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

This should be a relative link so it works offline

../core/panic/struct.PanicInfo.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@japaric japaric changed the title document #[panic_implementation] document #[panic_handler] Sep 7, 2018
@japaric
Copy link
Member Author

japaric commented Sep 7, 2018

Oh, I just noticed that CI fails because the example doesn't compile; I'll fix that up. Hmm, I was told in the reference PR that docs should not use feature gates but perhaps that's fine in the nomicon?

mdbook doesn't support no_std code
@japaric
Copy link
Member Author

japaric commented Sep 7, 2018

Err, I forgot that mdbook doesn't support no_std code so I have just marked the code snippets as ignored.

@steveklabnik steveklabnik merged commit 7fd4934 into rust-lang:master Sep 7, 2018
@steveklabnik
Copy link
Member

Awesome, thanks!

kennytm added a commit to kennytm/rust that referenced this pull request Sep 8, 2018
…imulacrum

stabilize #[panic_handler]

closes rust-lang#44489

### Update(2018-09-07)

This was proposed for stabilization in rust-lang#44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in rust-lang#44489 (comment)

Documentation PRs:

- Reference. rust-lang/reference#362
- Nomicon. rust-lang/nomicon#75

---

`#[panic_implementation]` was implemented recently in rust-lang#50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. rust-lang#44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in rust-lang#43054

Some unresolved questions from rust-lang#44489:

> Should the Display of PanicInfo format the panic information as "panicked at 'reason',
> src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

> Is this design compatible, or can it be extended to work, with unwinding implementations for
> no-std environments?

I believe @whitequark made more progress with unwinding in no-std since their last comment in rust-lang#44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp
bors added a commit to rust-lang/rust that referenced this pull request Sep 8, 2018
stabilize #[panic_handler]

closes #44489

### Update(2018-09-07)

This was proposed for stabilization in #44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in #44489 (comment)

Documentation PRs:

- Reference. rust-lang/reference#362
- Nomicon. rust-lang/nomicon#75

---

`#[panic_implementation]` was implemented recently in #50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. #44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in #43054

Some unresolved questions from #44489:

> Should the Display of PanicInfo format the panic information as "panicked at 'reason',
> src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

> Is this design compatible, or can it be extended to work, with unwinding implementations for
> no-std environments?

I believe @whitequark made more progress with unwinding in no-std since their last comment in #44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants