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.

Sign up for GitHub

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

Jump to bottom

Tweak std::marker docs #36424

Merged
merged 1 commit into from
Sep 16, 2016
Merged

Tweak std::marker docs #36424

merged 1 commit into from
Sep 16, 2016

Conversation

kmcallister
Copy link
Contributor

sfackler
sfackler reviewed Sep 12, 2016
View reviewed changes
src/libcore/marker.rs
@@ -60,13 +76,16 @@ pub trait Sized {
}

/// Types that can be "unsized" to a dynamically sized type.
///
/// For example, the sized array type `[i8; 2]` implements `Unsize<[u8]>` and
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth briefly talking about how Unsize is paired with CoerceUnsized and what it's used for here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's something I don't understand particularly well. Do you have a reference?

Copy link
Member

Choose a reason for hiding this comment

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

I think the RFC should still mostly reflect the current reality but I'm not super familiar with DST coercions either: /~https://github.com/nrc/rfcs/blob/4b99006cac67f7ad89c670bcb351d32bc0d5d33f/text/0000-dst-coercion.md

@frewsxcv
Copy link
Member 

Linkcheck stage2 (x86_64-unknown-linux-gnu)
core/marker/trait.Sync.html:98: broken link - core/mem/fn.transmute.html
thread 'main' panicked at 'found some broken links', src/tools/linkchecker/main.rs:54

frewsxcv
frewsxcv reviewed Sep 13, 2016
View reviewed changes
src/libcore/marker.rs
/// [rwlock]: ../../std/sync/struct.RwLock.html
/// [unsafecell]: ../cell/struct.UnsafeCell.html
/// [ub]: ../../reference.html#behavior-considered-undefined
/// [transmute]: ../mem/fn.transmute.html
Copy link
Member

Choose a reason for hiding this comment

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

core::mem::transmute doesn't exist, which might make this problematic?

Copy link
Member

Choose a reason for hiding this comment

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

Add "../std".

@kmcallister
Copy link
Contributor Author

Addressed review comments. Thanks everyone.

@GuillaumeGomez
Copy link
Member

Please squash your commits! Once done, I'd prefer to wait that #36363 has been merged before r+ yours to avoid conflicts.

@kmcallister
Copy link
Contributor Author

Yeah, that's going to conflict. I'll fix my PR once it lands.

@bors
Copy link
Contributor

bors commented Sep 14, 2016

☔ The latest upstream changes (presumably #36472) made this pull request unmergeable. Please resolve the merge conflicts.

@kmcallister kmcallister force-pushed the gh-29361-marker-docs branch 2 times, most recently from 58fafb7 to 81d3f6a Compare September 15, 2016 00:23
@kmcallister
Copy link
Contributor Author

I resolved conflicts and rebased.

GuillaumeGomez
GuillaumeGomez reviewed Sep 15, 2016
View reviewed changes
src/libcore/marker.rs
/// [typeid]: ../any/struct.TypeId.html
/// [param]: http://en.wikipedia.org/wiki/Parametricity
/// [size_of]: ../mem/fn.size_of.html
///
Copy link
Member

Choose a reason for hiding this comment

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

This line is useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/libcore/marker.rs
///
/// If your struct does not in fact *own* the data of type `T`, it is
/// better to use a reference type, like `PhantomData<&'a T>`
/// (ideally) or `PhantomData<*const T>` (if no lifetime applies), so
/// as not to indicate ownership.
///
/// [drop check]: ../../nomicon/dropck.html
///
Copy link
Member

Choose a reason for hiding this comment

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

This line is useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that some of the existing docs have it. I think it makes things more readable in the source code. If you feel very strongly I'll remove it, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Sep 15, 2016

📌 Commit b735c1b has been approved by GuillaumeGomez

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 15, 2016
Rollup merge of rust-lang#36424 - kmcallister:rust-langgh-29361-marke…
fbc1890 
…r-docs, r=GuillaumeGomez

Tweak std::marker docs

Fixes rust-lang#29361.

r? @GuillaumeGomez
@sophiajt sophiajt mentioned this pull request Sep 15, 2016
Closed
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 16, 2016
Rollup merge of rust-lang#36424 - kmcallister:rust-langgh-29361-marke…
d48f17b 
…r-docs, r=GuillaumeGomez

Tweak std::marker docs

Fixes rust-lang#29361.

r? @GuillaumeGomez
@sophiajt sophiajt mentioned this pull request Sep 16, 2016
Merged
bors added a commit that referenced this pull request Sep 16, 2016
@bors
Auto merge of #36536 - jonathandturner:rollup, r=jonathandturner
55a61a1 
Rollup of 8 pull requests

- Successful merges: #36383, #36424, #36480, #36484, #36505, #36509, #36519, #36521
- Failed merges:
@bors bors merged commit b735c1b into rust-lang:master Sep 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kmcallister @frewsxcv @GuillaumeGomez @bors @durka @sfackler