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

[NO MERGE] Comments from nostarch on ch 19 #1087

Closed
wants to merge 1 commit into from

Conversation

carols10cents
Copy link
Member

for review @steveklabnik

Copy link
Member Author

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

break

Closures. However, I'm not convinced that's ideal, so I thought we might
include a ToC at the top of this chapter in print so the reader can use it as a
reference when they come across something they can't figure out. What do you
think? -->
Copy link
Member Author

Choose a reason for hiding this comment

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

a table of contents should be fine, although wouldn't it be redundant with the table of contents at the beginning of the book? shrug

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i feel the same

skip this chapter and come back to it once you run into these things in the
wild; the features we’ll learn to use here are useful in very specific
about a few things you may run into that last 1% of the time. Feel free to skip
this chapter and come back to it only when you run into something unknown in
Copy link
Member Author

Choose a reason for hiding this comment

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

i don't like "only" here, you CAN read this chapter whenever you want

Copy link
Member

Choose a reason for hiding this comment

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

yes, agreed


People are fallible, and mistakes will happen, but by requiring these four
unsafe operations to be inside blocks annotated with `unsafe`, you’ll know that
any errors related to memory safety must be within this unsafe block of code.
Copy link
Member Author

Choose a reason for hiding this comment

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

some unsafe block of code, not any particular one (implied by "this")

have an immutable raw pointer and a mutable raw pointer, written as `*const T`
and `*mut T`, respectively. In the context of raw pointers, “immutable” means
that the pointer can’t be directly assigned to after being dereferenced.
Way back in Chapter 4, when first learned about references, we also learned
Copy link
Member Author

Choose a reason for hiding this comment

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

the "we" removed here needs to come back


Raw pointers are different than references and smart pointers in a few ways.
Raw pointers:
Different to references and smart pointers, raw pointers:
Copy link
Member Author

Choose a reason for hiding this comment

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

different than or different from or differently from or something, this sounds weird to me

Copy link
Member

Choose a reason for hiding this comment

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

I'd use "from"

- Don't implement any automatic clean-up

<!-- Can you say here what benefits these provide, over smart pointers and
references, and using the aspects in these bullets? -->
Copy link
Member Author

Choose a reason for hiding this comment

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

i'm not sure what to say the benefits are aside from opting out of the safety checks because you're trying to do something rust won't let you do

Copy link
Member

Choose a reason for hiding this comment

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

yup, that's basically it

raw pointers will be valid since we created them directly from references that
are guaranteed to be valid, but we can’t make that assumption about any raw
<!--So we create a raw pointer using the dereference operator? Is that the same
operator? Is it worth touching on why? -->
Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think it's the dereference operator, i was under the impression that *const and *mut were types? i could be wrong though, i've never actually stopped to think about this

Copy link
Member

Choose a reason for hiding this comment

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

it's not; *const T and *mut T are types. it's true that it uses an asterisk, but that's it.


We’ve created raw pointers by using `as` to cast an immutable and a mutable
reference into their corresponding raw pointer types: The `*const T` type
immutable, and `*mut T` is mutable. Because we created them directly from
Copy link
Member Author

Choose a reason for hiding this comment

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

the mutable/immutable stuff is now redundant because it was moved before the listing; what's left here is ungrammatical

methods, but they have an extra `unsafe` out front.

<!-- Above -- so what is the difference, when and why would we ever use the
unsafe function? -->
Copy link
Member Author

@carols10cents carols10cents Dec 30, 2017

Choose a reason for hiding this comment

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

i feel like the next sentence answers this, not sure why this is unclear? we could also add this sentence from the nomicon:

On functions, unsafe means that users of the function must check that function's documentation to ensure they are using it in a way that maintains the contracts the function requires.

well we do sort of say that below after showing the error message, maybe move that up here


#### `extern` Functions for Calling External Code are Unsafe
#### Using `extern` Functions to Call External Code
Copy link
Member Author

Choose a reason for hiding this comment

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

guess we had some extra spaces in this title, will fix

Copy link
Member Author

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

done reviewing

@@ -214,11 +231,11 @@ to use it properly, and we’ve verified that everything is correct.

#### Creating a Safe Abstraction Over Unsafe Code
Copy link
Member Author

Choose a reason for hiding this comment

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

on reread, this section feels like it's missing a transition, something like "just because a function contains unsafe code doesn't mean the whole function needs to be marked as unsafe. in fact, wrapping unsafe code in a safe function is a common abstraction. As an example, let's check out a function from the standard library..."

wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

`split_at_mut`, that requires some unsafe code and explore how we might
implement it. This safe method is defined on mutable slices: it takes one slice
and makes it into two by splitting the slice at the index given as an argument.
This is demonstrated in Listing 19-4:
Copy link
Member Author

Choose a reason for hiding this comment

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

i would replace the added "this" with "using split_at_mut is demonstrated...."

like Listing 19-5. For simplicity, we’re implementing `split_at_mut` as a
function rather than a method, and only for slices of `i32` values rather than
for a generic type `T`:
something like Listing 19-5, and will not compile. For simplicity, we’re
Copy link
Member Author

Choose a reason for hiding this comment

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

going to change "and will not compile" to "which will not compile"

is less than or equal to the length. The assertion means that if we pass an
index that’s greater than the length of the slice to split at, the function
will panic before it attempts to use that index.
index given as a parameter is within the slice by checking that its less than
Copy link
Member Author

Choose a reason for hiding this comment

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

it's

with the type `*mut i32`, which we’ve stored in the variable `ptr`.

We keep the assertion that the `mid` index is within the slice. Then we get to
the unsasfe code: the `slice::from_raw_parts_mut` takes a raw pointer and a
Copy link
Member Author

Choose a reason for hiding this comment

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

s/unsasfe/unsafe/

also either "the" before the function name needs to be removed or it needs to say "the slice::from_raw_parts_mut function"


```
let s1: str = "Hello there!";
let s2: str = "How's it going?";
```

<!-- Why do they need to have the same memory layout? Perhaps I'm not
understanding fully what is meant by the memory layout, is it worth explaining
that a little in this section? -->
Copy link
Member Author

Choose a reason for hiding this comment

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

how much memory to allocate. all types must take up the same amount of space

rule of dynamically sized types: we must always put values of dynamically sized
types behind a pointer of some kind.
`T` is located, a `&str` is *two* values: the address of the `str` and its
lengty. As such, a `&str` has a size we can know at compile time: it’s two
Copy link
Member Author

Choose a reason for hiding this comment

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

s/lengty/length/

in which dynamically sized types are used in Rust; they have an extra bit of
metadata that stores the size of the dynamic information. This leads us to the
golden rule of dynamically sized types: we must always put values of
dynamically sized types behind a pointer of some kind.

<!-- Note for Carol: `Rc<str>` is only in an accepted RFC right now, check on
its progress and pull this out if it's not going to be stable by Oct -->
Copy link
Member Author

Choose a reason for hiding this comment

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

woooo this made it into 1.21.0! rust-lang/rust#42565

implemented for everything the compiler knows the size of at compile time. In
addition, Rust implicitly adds a bound on `Sized` to every generic function.
That is, a generic function definition like this:
<!-- I think we dropped that one, right? -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

oh wait we dropped everything talking about Sized

case ‘f’ not to be confused with the `Fn` closure trait. `fn` is called a
*function pointer*. The syntax for specifying that a parameter is a function
pointer is similar to that of closures, as shown in Listing 19-34:
<!-- Maybe give an example of when we'd want to use this? -->
Copy link
Member Author

Choose a reason for hiding this comment

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

when you have a named function that you want to use someplace that takes something that implements an Fn trait, maybe move the map(ToString::to_string) example up here?

Closures. However, I'm not convinced that's ideal, so I thought we might
include a ToC at the top of this chapter in print so the reader can use it as a
reference when they come across something they can't figure out. What do you
think? -->
Copy link
Member

Choose a reason for hiding this comment

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

yeah, i feel the same

skip this chapter and come back to it once you run into these things in the
wild; the features we’ll learn to use here are useful in very specific
about a few things you may run into that last 1% of the time. Feel free to skip
this chapter and come back to it only when you run into something unknown in
Copy link
Member

Choose a reason for hiding this comment

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

yes, agreed

Furthermore, `unsafe` does not mean the code inside the block is necessarily
dangerous or that it will definitely have memory safety problems: the intent is
that you as the programmer will ensure the code inside an `unsafe` block will
have valid memory.
Copy link
Member

Choose a reason for hiding this comment

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

"have valid memory" is werid here

Copy link
Member Author

Choose a reason for hiding this comment

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

changing it to "the code inside an unsafe block will access memory in a valid way.", better?

Copy link
Member

Choose a reason for hiding this comment

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

yes

- Don't implement any automatic clean-up

<!-- Can you say here what benefits these provide, over smart pointers and
references, and using the aspects in these bullets? -->
Copy link
Member

Choose a reason for hiding this comment

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

yup, that's basically it


Raw pointers are different than references and smart pointers in a few ways.
Raw pointers:
Different to references and smart pointers, raw pointers:
Copy link
Member

Choose a reason for hiding this comment

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

I'd use "from"


<!-- have we discussed mangling before this? It doesn't ring a bell with me,
though it may have been in an early chapter that I forgot --- if not could you
give a quick explanation here? -->
Copy link
Member

Choose a reason for hiding this comment

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

it's fine. i'd rather not get into it either

We’ve managed to go this entire book without talking about *global variables*,
which Rust does support, but which can be problematic with Rust's owmership
rules: if you have two threads accessing the same mutable global variable, it
can incur a data race.
Copy link
Member

Choose a reason for hiding this comment

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

yeah i'd say cause

choose once what the type of `Item` will be, since there can only be one `impl
Iterator for Counter`. We don’t have to specify that we want an iterator of
`u32` values everywhere that we call `next` on `Counter`.

The benefit of not having to specify generic type parameters when a trait uses
Copy link
Member

Choose a reason for hiding this comment

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

i'm okay with defaulting to cutting things


### Supertraits to Use One Trait’s Functionality Within Another Trait
### Implementing Supertraits to Use One Trait’s Functionality Within Another Trait
Copy link
Member

Choose a reason for hiding this comment

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

agreed on both counts

inner type, if we used it directly to restrict the available functionality, for
example.

New types can also hide internal generic types. For example, we could provide a
Copy link
Member

Choose a reason for hiding this comment

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

its a single word, "newtypes"

@carols10cents
Copy link
Member Author

replaced by #1094

@carols10cents carols10cents deleted the nostarch-ch19 branch January 3, 2018 02:17
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.

2 participants