loop_break_value: add documentation for book #41857

dhardy · 2017-05-09T13:43:29Z

Some notes at the top of the file.

rust-highfive · 2017-05-09T13:43:36Z

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.

steveklabnik

This is great! Just a few comments.

steveklabnik · 2017-05-09T14:09:19Z

src/doc/unstable-book/src/language-features/loop-break-value.md

+Documentation to be appended to section 3.6 of the book: Loops (after "Loop Labels", or before if
+the "Break" section is moved). If this is deemed too complex a feature this early in the book, it
+could also be moved to a new section (please advise). This would allow examples breaking with
+non-primitive types, references, and discussion of coercion (probably unnecessary however).


So, our current strategy is that we have an appendix G, where stuff can land at first https://doc.rust-lang.org/nightly/book/second-edition/appendix-07-newest-features.html

I'd recommend saying this should go there instead

steveklabnik · 2017-05-09T14:09:35Z

src/doc/unstable-book/src/language-features/loop-break-value.md

 ------------------------

+### Loops as expressions
+
+Like everything else in Rust, loops are expressions; for example, the following is perfectly legal,


not everything is an expression; I'd soften this to "most things"

dhardy · 2017-05-09T14:55:39Z

Thanks!

CI complained a bit. Looks like I need to add #![feature(loop_break_value)] — but where? Each snippet?
And how do I run these tests locally?

Any idea on this — just a temporary failure?

And where should the reference changes land — here?

leoyvens

Thanks for writing this, it's very nice, I'm not an expert on docs or anything but left some comments with opinions.

Also I find it a bit confusing that through the text loop means any loop and loop is used meaning exactly the loop construct. Perhaps it would be interesting to rephrase using the word loop to mean only the loop construct?

leoyvens · 2017-05-09T15:54:59Z

src/doc/unstable-book/src/language-features/loop-break-value.md

+
+For now, breaking with a value is only possible with `loop`; the same functionality may
+some day be added to `for` and `while` (this would require some new syntax like
+`while f() { break 1; } default { break 0; }`).


I don't like that this speculates about future language extensions and their syntax.

I half agree with you here, but I think that if there is no mention of the discrepancy between loop and for/while that some readers will assume break-with-value also works there and confused why it doesn't work, and some will start asking the obvious why.

I suppose I could just omit the last bit about why for and while don't support break-with-value, but it still leaves an obvious question why.

The Rust book already uses loop to refer to all three repetitive control-flow constructs:

Rust has three kinds of loops: loop, while, and for.

Your concerns are valid, but the why response is complex and depends on who you ask, imo it's too opinionated to get into that here. I definitely think your use of the loop term is correct, I just worry it may be confusing in this context.

Yeah; we don't say stuff like this in the docs because it often bitrots; then people think you can't do it when you actually can!

leoyvens · 2017-05-09T16:16:10Z

src/doc/unstable-book/src/language-features/loop-break-value.md

+    println!("Hello, {}", n);
+};
+assert_eq!(result, ());
+```


I don't find this example very helpful in understanding the feature. The return value of for is a nice curiousity but I don't see much didatic value to this.

leoyvens · 2017-05-09T16:17:12Z

src/doc/unstable-book/src/language-features/loop-break-value.md

+```
+
+Until now, all the loops you have seen evaluate to either `()` or `!`, the latter being special
+syntax for "no value", meaning the loop never exits. A `loop` can instead evaluate to


I don't find the () vs ! distinction very useful, the meaning of ! as a type is unstable so maybe it's best to leave it out of this explanation.

…acas's comments

leoyvens · 2017-05-09T19:14:33Z

src/doc/unstable-book/src/language-features/loop-break-value.md

+### Loops as expressions
+
+Like most things in Rust, loops are expressions, and have a value; normally `()` unless the loop
+never exits.


Nice, I think this is more to the point now. "unless the loop never exits" only applies to loop and not any loop that never exists (see how even we are getting confused by the terms), for and while always evaluate to (). But diverging things are very niche, maybe we should just not mention this here?

steveklabnik · 2017-05-15T13:42:55Z

@dhardy

CI complained a bit. Looks like I need to add #![feature(loop_break_value)] — but where? Each snippet?

Yes, each snippet runs as its own test, so they all need them.

And how do I run these tests locally?

The simplest way is to run rustdoc --test on the file.

Any idea on this — just a temporary failure?

Yes, don't worry about that.

And where should the reference changes land — here?

Yup, it's in a separate repo.

carols10cents · 2017-05-15T15:06:58Z

CI complained a bit. Looks like I need to add #![feature(loop_break_value)] — but where? Each snippet?

Yep, in each snippet. See, for example, the docs on the used feature, #![feature(used)] is in each snippet.

And how do I run these tests locally?

You should be able to run just the unstable book tests by doing cd src/doc/unstable-book and then mdbook test (you might have to run cargo install mdBook to get the mdBook binary, not sure).

Any idea on this — just a temporary failure?

Yep, I've logged this in our spurious failure tracking. Let's see what happens when travis reruns after you fix the tests.

And where should the reference changes land — here?

Yep, that looks right!

carols10cents · 2017-05-15T15:07:18Z

AHHHH STEVE BEAT ME

carols10cents · 2017-05-15T20:10:05Z

connected to #37339

dhardy · 2017-05-16T17:56:30Z

Thanks Steve, Carols. Tests updated to pass, but I used no_compile on one; I'm not sure if there's a better way of handling should fail to compile examples?

steveklabnik · 2017-05-16T18:03:40Z

I used no_compile on one; I'm not sure if there's a better way of handling should fail to compile examples?

I uh, forgot no_compile existed 😅 sounds good to me

steveklabnik · 2017-05-17T15:11:58Z

@bors: r+

bors · 2017-05-17T15:11:59Z

📌 Commit 7ab35b7 has been approved by steveklabnik

bors · 2017-05-17T15:43:01Z

⌛ Testing commit 7ab35b7 with merge 7b5c3d2...

@steveklabnik

loop_break_value: add documentation for book Some notes at the top of the file. r? @steveklabnik

bors · 2017-05-17T18:38:58Z

☀️ Test successful - status-appveyor, status-travis
Approved by: steveklabnik
Pushing 7b5c3d2 to master...

loop_break_value: add documentation for book

52c3380

rust-highfive assigned steveklabnik May 9, 2017

steveklabnik suggested changes May 9, 2017

View reviewed changes

loop_break_value: address review comments on book addition

7d94b48

leoyvens reviewed May 9, 2017

View reviewed changes

loop_break_value book doc: remove some curiosities, regarding leodasv…

7488ff5

…acas's comments

leoyvens reviewed May 9, 2017

View reviewed changes

TimNN added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 10, 2017

loop_break_value: fix tests (but ignore one expected not to compile)

3f980be

loop_break_value doc: remove note about other loops

7ab35b7

steveklabnik mentioned this pull request May 17, 2017

Document the loop_break_value feature rust-lang/book#699

Merged

steveklabnik approved these changes May 17, 2017

View reviewed changes

bors added a commit that referenced this pull request May 17, 2017

Auto merge of #41857 - dhardy:master, r=steveklabnik

7b5c3d2

loop_break_value: add documentation for book Some notes at the top of the file. r? @steveklabnik

bors merged commit 7ab35b7 into rust-lang:master May 17, 2017

bors mentioned this pull request May 17, 2017

Stabilize the loop_break_value feature #42016

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loop_break_value: add documentation for book #41857

loop_break_value: add documentation for book #41857

dhardy commented May 9, 2017

rust-highfive commented May 9, 2017

steveklabnik left a comment

steveklabnik May 9, 2017

steveklabnik May 9, 2017

dhardy commented May 9, 2017

leoyvens left a comment

leoyvens May 9, 2017

dhardy May 9, 2017 •

edited

Loading

dhardy May 9, 2017

leoyvens May 9, 2017

steveklabnik May 11, 2017

leoyvens May 9, 2017 •

edited

Loading

leoyvens May 9, 2017

leoyvens May 9, 2017

steveklabnik commented May 15, 2017

carols10cents commented May 15, 2017

carols10cents commented May 15, 2017

carols10cents commented May 15, 2017

dhardy commented May 16, 2017

steveklabnik commented May 16, 2017

steveklabnik commented May 17, 2017

bors commented May 17, 2017

bors commented May 17, 2017

bors commented May 17, 2017

loop_break_value: add documentation for book #41857

loop_break_value: add documentation for book #41857

Conversation

dhardy commented May 9, 2017

rust-highfive commented May 9, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented May 9, 2017

leoyvens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoyvens May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented May 15, 2017

carols10cents commented May 15, 2017

carols10cents commented May 15, 2017

carols10cents commented May 15, 2017

dhardy commented May 16, 2017

steveklabnik commented May 16, 2017

steveklabnik commented May 17, 2017

bors commented May 17, 2017

bors commented May 17, 2017

bors commented May 17, 2017

dhardy May 9, 2017 •

edited

Loading

leoyvens May 9, 2017 •

edited

Loading