-
Notifications
You must be signed in to change notification settings - Fork 489
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
Generic parameters #258
Generic parameters #258
Conversation
src/items/enumerations.md
Outdated
[_Generics_]: items.html#type-parameters | ||
[_WhereClause_]: items.html#type-parameters | ||
[_Generics_]: items/generics.html | ||
[_WhereClause_]: iitems/generics.html#where-clauses |
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.
iitems -> items
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 didn't finish this review, but I have to go to work. I really just reviewed items/generics.md before the Where Clauses section.
Overall, this is high quality work, and I'm just found editing things.
src/items/generics.md
Outdated
> _LifetimeParam_ (`,` _LifetimeParam_)<sup>\*</sup> | ||
> | ||
> _LifetimeParam_ : | ||
> [LIFETIME_OR_LABEL] [_LifetimeBounds_]<sup>?</sup> |
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.
You want a
or "\ " (I think?) between the links or else markdown is going to think the first link is actually the text for the second link.
src/items/generics.md
Outdated
> _TypeParam_ (`,` _TypeParam_)<sup>\*</sup> | ||
> | ||
> _TypeParam_ : | ||
> [IDENTIFIER] [_TypeParamBounds_]<sup>?</sup> ( `=` [_Type_] )<sup>?</sup> |
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.
Likewise here with spaces and links.
src/items/generics.md
Outdated
|
||
Functions, type aliases, structs, enumerations, unions, traits and | ||
implementations may be *parameterized* by types and lifetimes. These parameters | ||
are listed in angle brackets (`<...>`), after the name of the item (except for |
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.
For the angle brackets parenthesis, you'll want to wrap it and the word before it in a <span class="parenthetical">
.
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.
For the second parenthetical, is there a way to write it without using parentheticals at all?
src/items/generics.md
Outdated
``` | ||
|
||
Certain built in types: [references], [raw pointers], [arrays], | ||
[slices][arrays], [tuples] and [function pointers], have lifetime or type |
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.
Nit: built-in*
Nit: trailing comma after "function pointers"
Less nitty: Is the prefix of saying they are built-in types necessary here? The text would flow more smoothly if you just started at "References", IMO.
src/items/generics.md
Outdated
|
||
Certain built in types: [references], [raw pointers], [arrays], | ||
[slices][arrays], [tuples] and [function pointers], have lifetime or type | ||
parameters as well, but use different syntax. |
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.
Nit: "use a 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.
Works done. Here's the rest of the review.
I'm basically 😍 looking at this. The content is fantastic.
src/items/generics.md
Outdated
> [_Lifetime_](#type-and-lifetime-parameters) [_LifetimeBounds_] | ||
> | ||
> _TypeBoundWhereClauseItem_ : | ||
> _ForLifetimes_<sup>?</sup> [_Type_] `:` [_TypeParamBounds_] |
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 feel like ForLifetimes should be defined in this section for now, even if we move it later.
src/items/generics.md
Outdated
> _TypeBoundWhereClauseItem_ : | ||
> _ForLifetimes_<sup>?</sup> [_Type_] `:` [_TypeParamBounds_] | ||
|
||
Where clauses provide an alternative way to specify bounds on type and lifetime |
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.
Nit: Where clauses should be italicized. I've used *
for this myself, and others have used _
.
src/items/generics.md
Outdated
> _ForLifetimes_<sup>?</sup> [_Type_] `:` [_TypeParamBounds_] | ||
|
||
Where clauses provide an alternative way to specify bounds on type and lifetime | ||
parameters, as well as a way to specify bounds on types that aren't type |
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.
Nit: English is weird, but I don't think that comma needs to be there.
draw_twice(surface, sh2); // Can call this since U: Shape | ||
} | ||
``` | ||
Generic items may use traits as [bounds] on their type parameters. |
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.
If you're going to gut this section, then fold this into another section somehow.
src/items/generics.md
Outdated
> _TypeBoundWhereClauseItem_ : | ||
> _ForLifetimes_<sup>?</sup> [_Type_] `:` [_TypeParamBounds_] | ||
|
||
Where clauses provide an alternative way to specify bounds on type and lifetime |
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 would get rid of the word "alternative" as it makes it feel like trait bounds in the type parameter lists is the primary way.
src/trait-bounds.md
Outdated
|
||
## Lifetime bounds | ||
|
||
Lifetime bounds can be applied to other lifetimes, or to types. The bound |
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.
Nit: to types or other lifetimes
src/trait-bounds.md
Outdated
|
||
Lifetime bounds can be applied to other lifetimes, or to types. The bound | ||
`'a: 'b` is usually read as `'a` *outlives* `'b`. `'a: 'b` means that `'a` | ||
includes all points in the code that `'b` does. |
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.
A bit fuzzy here, but we don't really describe what a point in the code is.
src/trait-bounds.md
Outdated
|
||
> **<sup>Syntax</sup>** | ||
> _ForLifetimes_ : | ||
> `for` `<` ( [_LifetimeParams_][generic][^unused_for_bounds] |
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.
The same issue is happening here with the [^unused_for_bounds]
thing. I think the grammar is busy enough without footnotes distracting you away from it.
src/trait-bounds.md
Outdated
|
||
Type bounds may be *higher ranked* over lifetimes. A bound such as `for<'a> &'a | ||
T: PartialEq<T>` can be used to show `&'a T: PartialEq<T>` for any lifetime. | ||
For example, only a higher-ranked bound can be used here as the lifetime of the |
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.
Nit: The "For example" should be placed on the previous sentence.
src/trait-bounds.md
Outdated
> `for` `<` ( [_LifetimeParams_][generic][^unused_for_bounds] | ||
> `,`<sup>?</sup> ) <sup>?</sup> `>` | ||
|
||
Type bounds may be *higher ranked* over lifetimes. A bound such as `for<'a> &'a |
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.
Given that the rest of this section is either examples or the scope of the bound, there's not much here explaining abstractly what higher ranked lifetimes are.
80000d4
to
ffd1096
Compare
Comments addressed, I think? |
src/types.md
Outdated
@@ -334,7 +334,7 @@ let foo_ptr_2 = if want_i32 { | |||
}; | |||
``` | |||
|
|||
All function items implement [Fn], [FnMut], [FnOnce], [Copy], [Clone], [Send], | |||
All function items implement [Fn], [FnMut], [FnOnce], [Copy], [Clone], [Send], |
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.
These should be in teletype.
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 have one very minor nit, it looks good to me otherwise but I think I'd like to have @Havvy do another pass since he did the first review.
e82338e
to
642ae9d
Compare
rebased, but CI still can't build mdbook. |
Is it possible to rerun CI? |
642ae9d
to
00d5651
Compare
Pushed another commit, CI hasn't changed. |
The tests are passing on my computer, and TravisCI is acting weird, so merging as is. |
Putting this up to get some feedback. Second PR contains all the major changes.
cc #256