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

Documentation PR for cfg_panic #1157

Merged
merged 10 commits into from
Mar 15, 2022
Merged

Documentation PR for cfg_panic #1157

merged 10 commits into from
Mar 15, 2022

Conversation

cchiw
Copy link
Contributor

@cchiw cchiw commented Feb 4, 2022

@cchiw cchiw mentioned this pull request Feb 4, 2022
Closed
3 tasks
bjorn3
bjorn3 reviewed Feb 5, 2022
View reviewed changes
src/attributes/testing.md Outdated
## Different panic strategies

The `cfg_panic` feature makes it possible to exercise different lines of code depending on the panic strategy.
The possible value is either `unwind` or `abort`.
Copy link
Member

Choose a reason for hiding this comment

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

Should this say make clear that code compiled with #[cfg(panic = "unwind")] being true may still aborting on panic if any crate is compiled with -Cpanic=abort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good thing to point out. Note, I'm not as familiar with crates so i don't know too much about this but I'll take your word for it.

I updated the file, let me know what you think :)

bjorn3
bjorn3 reviewed Feb 7, 2022
View reviewed changes
src/attributes/testing.md Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented Feb 9, 2022

Thanks for the PR! I think this might need to go in a different direction, though. The cfg options are documented in the Conditional Compilation chapter. I would expect this to just add a new section similar to target_pointer_width. Also, I wouldn't bother with an example, since that chapter already has some examples.

@Manishearth Manishearth mentioned this pull request Feb 10, 2022
Merged
@cchiw
Copy link
Contributor Author

cchiw commented Mar 9, 2022

Hi, just following up. How would you like to proceed on this PR? Should I adjust it or would you like to not include it? The issue is still open pending this thread.

Just FYI, the compiler changes will be accepted to version 1.60, the Rust by Example documentation was merged in, and the Rust Book chose not to include it.

@ehuss
Copy link
Contributor

ehuss commented Mar 9, 2022

Can you update the PR to instead add a section in the Conditional Compilation chapter? There are several sections there with examples of how cfg values are documented, and you can follow that format for adding cfg_panic. Thanks!

@cchiw
Copy link
Contributor Author

cchiw commented Mar 14, 2022

OK. I added an example to the Conditional Compilation chapter and removed the additional text from the Attributes-Testing section. Let me know what you think.

@ehuss
Copy link
Contributor

ehuss commented Mar 14, 2022

Sorry if I wasn't clear. The documentation should look like the other sections in the conditional compilation chapter. For example, there should be a section (probably just under the proc_macro one) that looks roughly like this:

panic

Key-value option set for …

Example values:

  • "abort"
  • "unwind"

Also, the example using #[cfg(not(panic="unwind"))] is using a form that is assuming there are only two panic strategies. There is a note here where it was stated that we should explicitly avoid that. I think the text here should very briefly note that other values may be added in the future.

@cchiw
Copy link
Contributor Author

cchiw commented Mar 14, 2022 •
edited
Loading

Ah, OK. I added the suggested section. Thank you for the feedback.

ehuss
ehuss approved these changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss merged commit 0a2fe66 into rust-lang:master Mar 15, 2022
@ehuss ehuss mentioned this pull request Mar 16, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2022
@bors
Auto merge of rust-lang#94990 - ehuss:update-books, r=ehuss
d8e5647 
Update books

## reference

8 commits in 9d289c05fce7254b99c6a0d354d84abb7fd7a032..0a2fe6651fbccc6416c5110fdf5b93fb3cb29247
2022-02-23 08:58:20 -0800 to 2022-03-15 09:32:25 -0700
- Documentation PR for cfg_panic (rust-lang/reference#1157)
- Document aarch64 `target_feature` options (rust-lang/reference#1102)
- Try to clarify destructor not being run scenario. (rust-lang/reference#1107)
- Add undocumented Punctuation token Tilde `~` (rust-lang/reference#1149)
- update UB list for safe target_feature (rust-lang/reference#1050)
- Update const_eval.md for feature stabilization (rust-lang/reference#1166)
- Remove `.intel_syntax`/`.att_syntax` support entirely.
- Fix `.intel_syntax` directive

## book

3 commits in 3f255ed40b8c82a0434088568fbed270dc31bf00..036e88a4f135365de85358febe5324976a56030a
2022-02-27 21:26:12 -0500 to 2022-03-04 21:53:33 -0500
- Fix some links and small wordings
- Snapshot of chapter 19 for nostarch
- Clarify fully-qualified syntax explanation

## rust-by-example

2 commits in 2a928483a20bb306a7399c0468234db90d89afb5..d504324f1e7dc7edb918ac39baae69f1f1513b8e
2022-02-28 11:36:59 -0300 to 2022-03-07 09:26:32 -0300
- Fixed extra indentation at line 43 in Phantom Testcase example. (rust-lang/rust-by-example#1515)
- Typo fixed in description of inline ASM cpuid function (rust-lang/rust-by-example#1514)

## rustc-dev-guide

3 commits in 32f2a5b..0e4b961
2022-03-01 10:45:24 -0600 to 2022-03-14 08:40:37 -0700
- update winget install instructions to ensure proper packages are installed (-e for --exact, and full package names to ensure arbitrary packages from
the msstore source aren't installed)
- Add missing rustdoc tests explanations
- Fix incorrectly escaped backtick
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@ehuss ehuss ehuss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cchiw @ehuss @bjorn3