-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Rewrote "How Safe and Unsafe Interact" Nomicon chapter. #33895
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Here's the current version of the rewritten chapter, for convenience. |
Unsafe Rust code cannot assume that any `Ord` implementation it gets makes | ||
sense. The unsafe portions of `BTreeMap`'s internals have to be careful to | ||
maintain all necessary contracts, even if a key type's `Ord` implementation | ||
is completely nonsensical. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be more specific here, and say "even if a key type's Ord implementation does not implement a total ordering"
please squash |
The previous version of the chapter covered a lot of ground, but was a little meandering and hard to follow at times. This draft is intended to be clearer and more direct, while still providing the same information as the previous version.
Alright, I've squashed. |
Hey, sorry for taking a while to review this. Will try to get to it tomorrow. |
No problem! |
@AndrewBrinker so, since this is a pretty significant re-write, and it's hard to tell what's different and what's changed, I'm going to review this from the position of a brand-new chapter. Some of which, you inherited, of course :) If you don't have the time/desire to make these kinds of tweaks, let me know, and we don't have to block the PR on them, but I might need your help in separating the past from the present, given the diff is so unhelpful here. 😄 |
interact? | ||
|
||
The separation between Safe Rust and Unsafe Rust is controlled with the | ||
`unsafe` keyword, which acts as a sort of *foreign function interface* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little uncomfortable with overloading this term in this way. It's not the worst, but I wonder if we can not make this implication so strong. Then again, maybe it is apt; unsafe
is a part of the functions' type, so in some sense, they are different... i dunno.
The reason I'm a bit hesitant is this invokes something like "wait, is the calling convention different?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit of an overloaded term in this context. I am happy to take a crack at a different wording tonight.
I've rewritten the second paragraph to eliminate the FFI bits, and I think it's actually clearer now. I also made the smaller changes you suggested. As far as what's different between this and the original, I'll start by noting that my goal with this was basically to reword and simplify the original, so there are a lot of similarities. The first paragraph ("What's the relationship...") is the same in both versions. The second paragraph in the new version ("The separation between...") is a combination of paragraphs 2 and 3 from the original, with the FFI stuff removed. Paragraphs 3, 4, 5, and 6 in the new version ("The Paragraphs 7, 8, 9, and 10 in the new version ("The standard library has..." to " Paragraph 11 in the new version ("Much of the Rust standard library...") is pulled from paragraph 13 in the original. This just explains that the use of unsafe in the standard library isn't a cause for alarm. Paragraphs 12 and 13 in the new version ("The need for all this..." and "No matter what...") are a rewording of paragraphs 11 and 12 in the original. These introduce probably the biggest point in the piece, the core idea that differentiates Safe Rust and Unsafe Rust: that Safe Rust can't cause undefined behavior. Paragraphs 14-18 in the new version ("The design of the...") are a reworking of paragraphs 13-21 in the original. These provide greater detail about when to use unsafe, and how unsafe code must be written not to rely on safe code. Paragraph 19 in the new version ("The decision of whether...") is a reworking of paragraphs 21-23 in the original. This tries to explain that use of Paragraphs 20 and 21 in the new version are new, and not adapted from the original. Paragraph 20 clarifies how Hope that helps! |
@bors: r+ rollup thanks a ton! |
📌 Commit af33b30 has been approved by |
😄 |
Rewrote "How Safe and Unsafe Interact" Nomicon chapter. The previous version of the chapter covered a lot of ground, but was a little meandering and hard to follow at times. This draft is intended to be clearer and more direct, while still providing the same information as the previous version. r? @steveklabnik
Rewrote "How Safe and Unsafe Interact" Nomicon chapter. The previous version of the chapter covered a lot of ground, but was a little meandering and hard to follow at times. This draft is intended to be clearer and more direct, while still providing the same information as the previous version. r? @steveklabnik
The previous version of the chapter covered a lot of ground, but was a little meandering and hard to follow at times. This draft is intended to be clearer and more direct, while still providing the same information as the previous version.
r? @steveklabnik