Skip to content

Show impl<T> and explain it for trait bounds and operators #27285

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

Merged
merged 3 commits into from
Aug 5, 2015

Conversation

lastorset
Copy link
Contributor

I also included some smaller trait-related changes.

Fixes #26991.

r? @shepmaster
r? @steveklabnik

@@ -57,7 +57,7 @@ enum Result<T, E> {
```

This type is generic over _two_ types: `T` and `E`. By the way, the capital letters
can be any letter you’d like. We could define `Result<T, E>` as:
can be any letter or word you’d like. We could define `Result<T, E>` as:
Copy link
Member

Choose a reason for hiding this comment

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

While this is true, type parameters are virtually always a single letter.

@steveklabnik
Copy link
Member

Looking great! Just have some nits. Would you mind squashing this too please?

@lastorset
Copy link
Contributor Author

@steveklabnik Thanks for the review! I'll fix the issues you found.

Do you want it all current commits squashed into one commit (or two: new material and edits), or just my fixes to your review comments squashed into the current commits? Do I need to submit a new PR for the squashed branch, or can I force push to this one? (Haven't done this on GitHub much, I'm used to Critic. :))

@lastorset
Copy link
Contributor Author

Documentation for another project implies that I can force push into this PR. So that's one question answered. (I do wonder what happens to the review comments. Probably GitHub is smart enough to keep them and the original commit objects around.)

This includes a new example with Rectangle, instead of reusing HasArea,
because fn area would require the Mul trait, and the added complexity of
that would be better left for the Operators and Overloading chapter.

Squashed at reviewer's request: Move teaser for trait bounds to bottom
Squashed at reviewer's request:

Add heading at the end of the introductory material
Spice up introductory paragraphs a bit
Use quotes instead of <code> for phrase
Remove "other" in "other restrictions" (it's not obvious that any other
restrictions have been mentioned)
"Default methods" is a second-level heading, but is not a subsection of
"Where clause"
Reword "Default methods" introduction: it's not the "last feature" on
this page
@lastorset lastorset force-pushed the trait-operator-impl branch from 1ca10ce to 4277369 Compare July 30, 2015 20:18
@lastorset
Copy link
Contributor Author

It was unclear how exactly I should squash, but I think a reasonable interpetration would be to keep the new material in separate commits and squash down all the copyedits. I push a separate branch where I amended the original commits; you can merge that if you want more cohesive commits.

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Collaborator

bors commented Aug 5, 2015

📌 Commit 4277369 has been approved by steveklabnik

@bors
Copy link
Collaborator

bors commented Aug 5, 2015

⌛ Testing commit 4277369 with merge 5fc614b...

@steveklabnik
Copy link
Member

hey sorry i missed this! You can squash into one or into more logical commits, like you've done here. thanks so much!

@bors
Copy link
Collaborator

bors commented Aug 5, 2015

💔 Test failed - auto-win-gnu-32-nopt-t

@Gankra
Copy link
Contributor

Gankra commented Aug 5, 2015

@bors retry

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Aug 5, 2015
…eveklabnik

I also included some smaller trait-related changes.

Fixes rust-lang#26991.

r? @shepmaster 
r? @steveklabnik
bors added a commit that referenced this pull request Aug 5, 2015
@bors bors merged commit 4277369 into rust-lang:master Aug 5, 2015
@lastorset
Copy link
Contributor Author

No problem, thank you too!

@lastorset lastorset deleted the trait-operator-impl branch August 6, 2015 09:06
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.

4 participants