-
Notifications
You must be signed in to change notification settings - Fork 13.9k
TRPL: dining philosophers #25321
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
TRPL: dining philosophers #25321
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
oh, and i might not need the channel at the end: https://twitter.com/florob/status/597935275394912259 considering an extra 'cleanup' section |
src/doc/trpl/dining-philosophers.md
Outdated
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.
"has is" -> "is"
e6a6ce0 to
618ef96
Compare
|
I believe all nits are addressed. r? @alexcrichton |
|
So, maybe I'm just terribly dense, but I still don't see why you need the channel at all. AFAICT, it does not become obsolete by joining the threads, but is unnecessary from the get go. |
src/doc/trpl/dining-philosophers.md
Outdated
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'm still kinda worried about all these .ok().expect(...) calls throughout these examples, some of them I think may be giving the wrong impression. For example this doesn't really indicate anything about "couldn't finish eating" but rather that no one's listening for the notification that the eating is done. Down below there's:
.ok().expect("Couldn't aquire left mutex");If Err is returned, though, the mutex was indeed acquired, it was just poisoned.
Would it be possible to just use unwrap everywhere? I would consider unwrap as the idiomatic way to indicate that an error should not ever be happening here, and errors should definitely not ever be happening in these examples.
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 guess I've seen people mostly argue that unwrap gives a poor error message, and ok/expect at least lets you customize it.
that said, I'm sympathetic to the last thing you've said, so yeah, let me re-work it.
|
I do also agree with @Florob that the channel involvement here may not be pulling its weight by the time we get to the end. |
|
I added a thing at the end about removing the channel entirely. |
|
@Florob yeah, maybe that is true. sigh. |
618ef96 to
cde1dd6
Compare
|
Okay, I removed the channels entirely, and use unwrap. |
src/doc/trpl/dining-philosophers.md
Outdated
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.
"and report back that they're done", perhaps drop this phrase?
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.
nice catch. editing your own prose is so hard :(
src/doc/trpl/dining-philosophers.md
Outdated
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.
This may need to be rephrased as it's not just a plain vector instead of an array.
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.
👍
|
Looks good to me! A few last minor nits and otherwise r=me Nice chapter @steveklabnik! |
cde1dd6 to
2ba6169
Compare
|
@bors: r=alexcrichton rollup |
|
📌 Commit 2ba6169 has been approved by |
…xcrichton This is a little rough, and it needs squashed and section headers, but i'd like to get some eyes on it sooner rather than later.
…xcrichton This is a little rough, and it needs squashed and section headers, but i'd like to get some eyes on it sooner rather than later.
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.
Blegh, I think this is already being tested in a rollup, but maybe there will be a quick chance to fix a couple things soon.
s/mult-threading/multi-threading/
Edit: Never mind, I was missing a paragraph. This is the only nit I found.
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'll make a new PR for master, and work it into my backport. Thanks!
This is a little rough, and it needs squashed and section headers, but i'd like to get some eyes on it sooner rather than later.