Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1e493fd
Add explanations about what derived trait implementations do
carols10cents May 20, 2016
bbfb6e7
`derive` explanation for PartialOrd should match that for Ord
carols10cents May 21, 2016
2f44053
Make `Derivable` header be an h2 instead of an h1
carols10cents May 21, 2016
fc467b3
Reorder `Copy` doc sections
carols10cents May 21, 2016
e831c72
Add an explicit "How can I implement `PartialEq`"? doc section
carols10cents May 21, 2016
54d2ef0
Add an explicit "How can I implement `Eq`" doc section
carols10cents May 21, 2016
c41227f
Add more detail to `Clone`'s documentation
carols10cents May 22, 2016
61bb9b2
Add more information about implementing `Hash`
carols10cents May 22, 2016
9efa445
Add an explicit "How can I implement `Ord`" doc section
carols10cents May 22, 2016
8b00a08
Add an explicit "How can I implement `PartialOrd`" doc section
carols10cents May 22, 2016
bd50eff
Make the Default docs more like the other traits
carols10cents May 22, 2016
b4e123d
Shorten, yet clarify, initial summary sentences
carols10cents May 23, 2016
d2ee6e0
Emphasize semantic differences of Copy/Clone rather than impl
carols10cents May 23, 2016
c22c524
"more than 32" => "more than 32 elements"
carols10cents May 23, 2016
497cbb6
"non equal" => "not equal"; consistent with the surrounding text
carols10cents May 23, 2016
9149992
Add some newlines in some code examples
carols10cents May 23, 2016
daa9dca
Use `()` when referring to functions
carols10cents May 23, 2016
d81a999
Prefer `ClassName` over `Self` in example trait implementations
carols10cents May 23, 2016
1b32298
Move all `Default` docs from module to trait
carols10cents May 23, 2016
1a7d3e1
Complete `PartialOrd`'s example so it passes make check-docs
carols10cents May 23, 2016
1e809f5
"the trait `Hash`" => "the `Hash` trait"
carols10cents May 23, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions src/libcore/clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,39 @@

use marker::Sized;

/// A common trait for cloning an object.
/// A common trait for cloning an object. Differs from `Copy` in that you can
/// define `Clone` to run arbitrary code, while you are not allowed to override
/// the implementation of `Copy` that only does a `memcpy`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, GitHub is being weird, and when I refreshed, my comments were gone, so trying again. Sorry if this is a duplicate.

First, this summary line is a bit long. We try to keep the very first line a sentence, and while what was here was a bit... sparse, the summary line should still be shorter than this.

Second, the Copy/Clone distinction isn't really about overriding, exactly. Or rather, to me, the not-overriding is the implementation rather than the important bit. The important bit is that Copy is extremely inexpensive and is implicit. Clone may or may not be expensive, and is always explicit. The mechanism to ensure this is to not let you implement the details of a Copy.

I also have small concerns about putting stuff about Copy's semantics right here in the summary; while they won't really be changing.... idk. Hm.

Copy link
Member Author

Choose a reason for hiding this comment

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

I love your explanation of the distinction between Clone and Copy, working that in.

///
/// This trait can be used with `#[derive]`.
/// Since `Clone` is more general than `Copy`, you can automatically make anything
/// `Copy` be `Clone` as well.
///
/// ## Derivable
///
/// This trait can be used with `#[derive]` if all fields are `Clone`. The `derive`d
/// implementation of `clone()` calls `clone()` on each field.
///
/// ## How can I implement `Clone`?
///
/// Types that are `Copy` should have a trivial implementation of `Clone`. More formally:
/// if `T: Copy`, `x: T`, and `y: &T`, then `let x = y.clone();` is equivalent to `let x = *y;`.
/// Manual implementations should be careful to uphold this invariant; however, unsafe code
/// must not rely on it to ensure memory safety.
///
/// An example is an array holding more than 32 of a type that is `Clone`; the standard library
Copy link
Contributor

Choose a reason for hiding this comment

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

"more than 32 elements"

/// only implements `Clone` up until arrays of size 32. In this case, the implementation of
/// `Clone` cannot be `derive`d, but can be implemented as:
///
/// ```
/// #[derive(Copy)]
/// struct Stats {
/// frequencies: [i32; 100],
/// }
///
/// impl Clone for Stats {
/// fn clone(&self) -> Self { *self }
/// }
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub trait Clone : Sized {
/// Returns a copy of the value.
Expand Down
145 changes: 141 additions & 4 deletions src/libcore/cmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,42 @@ use option::Option::{self, Some};
/// symmetrically and transitively: if `T: PartialEq<U>` and `U: PartialEq<V>`
/// then `U: PartialEq<T>` and `T: PartialEq<V>`.
///
/// ## Derivable
///
/// This trait can be used with `#[derive]`. When `derive`d on structs, two
/// instances are equal if all fields are equal, and non equal if any fields
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about s/non/not/, to make it consistent with the rest of the paragraph?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oooh yep. That's what I get for copy pasting without revising.

/// are not equal. When `derive`d on enums, each variant is equal to itself
/// and not equal to the other variants.
///
/// ## How can I implement `PartialEq`?
///
/// PartialEq only requires the `eq` method to be implemented; `ne` is defined
/// in terms of it by default. Any manual implementation of `ne` *must* respect
/// the rule that `eq` is a strict inverse of `ne`; that is, `!(a == b)` if and
/// only if `a != b`.
///
/// This trait can be used with `#[derive]`.
/// An example implementation for a domain in which two books are considered
/// the same book if their ISBN matches, even if the formats differ:
///
/// ```
/// enum BookFormat { Paperback, Hardback, Ebook }
/// struct Book {
/// isbn: i32,
/// format: BookFormat,
/// }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: newline after this line, to give it some room

/// impl PartialEq for Book {
/// fn eq(&self, other: &Self) -> bool {
/// self.isbn == other.isbn
/// }
/// }
///
/// let b1 = Book { isbn: 3, format: BookFormat::Paperback };
/// let b2 = Book { isbn: 3, format: BookFormat::Ebook };
/// let b3 = Book { isbn: 10, format: BookFormat::Paperback };
///
/// assert!(b1 == b2);
/// assert!(b1 != b3);
/// ```
///
/// # Examples
///
Expand Down Expand Up @@ -96,7 +126,32 @@ pub trait PartialEq<Rhs: ?Sized = Self> {
/// This property cannot be checked by the compiler, and therefore `Eq` implies
/// `PartialEq`, and has no extra methods.
///
/// This trait can be used with `#[derive]`.
/// ## Derivable
///
/// This trait can be used with `#[derive]`. When `derive`d, because `Eq` has
/// no extra methods, it is only informing the compiler that this is an
/// equivalence relation rather than a partial equivalence relation. Note that
/// the `derive` strategy requires all fields are `PartialEq`, which isn't
/// always desired.
///
/// ## How can I implement `Eq`?
///
/// If you cannot use the `derive` strategy, specify that your type implements
/// `Eq`, which has no methods:
///
/// ```
/// enum BookFormat { Paperback, Hardback, Ebook }
/// struct Book {
/// isbn: i32,
/// format: BookFormat,
/// }
/// impl PartialEq for Book {
/// fn eq(&self, other: &Self) -> bool {
/// self.isbn == other.isbn
/// }
/// }
/// impl Eq for Book {}
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub trait Eq: PartialEq<Self> {
// FIXME #13101: this method is used solely by #[deriving] to
Expand Down Expand Up @@ -190,8 +245,48 @@ impl Ordering {
/// - total and antisymmetric: exactly one of `a < b`, `a == b` or `a > b` is true; and
/// - transitive, `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
///
/// ## Derivable
///
/// This trait can be used with `#[derive]`. When `derive`d, it will produce a lexicographic
/// ordering based on the top-to-bottom declaration order of the struct's members.
///
/// ## How can I implement `Ord`?
///
/// `Ord` requires that the type also be `PartialOrd` and `Eq` (which requires `PartialEq`).
///
/// Then you must define an implementation for `cmp`. You may find it useful to use
Copy link
Contributor

Choose a reason for hiding this comment

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

I try to include the ()s when referring to functions.

/// `cmp` on your type's fields.
///
/// Here's an example where you want to sort people by height only, disregarding `id`
/// and `name`:
///
/// ```
/// use std::cmp::Ordering;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a newline after this

/// #[derive(Eq)]
/// struct Person {
/// id: u32,
/// name: String,
/// height: u32,
/// }
///
/// impl Ord for Person {
/// fn cmp(&self, other: &Self) -> Ordering {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a style thing here that I'm not totally sure on; I would write &Person here, not &Self.

@aturon what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think I kept it Self because the trait signature uses it, but it has to and this doesn't have to.

I feel like preferring self over the class name this is a style thing I got from Greg Brown, with the reasoning that if you change the class name, you'd have fewer places to change. But I don't feel super strongly about it because 1. how often do you change class names anyway and 2. shouldn't your tools (find and replace at least, the compiler, IDE refactoring tools, etc) be handling this for you... so I'm adding a commit changing &Self to &Person here :)

/// self.height.cmp(&other.height)
/// }
/// }
///
/// impl PartialOrd for Person {
/// fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
/// Some(self.cmp(other))
/// }
/// }
///
/// impl PartialEq for Person {
/// fn eq(&self, other: &Self) -> bool {
/// self.height == other.height
/// }
/// }
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub trait Ord: Eq + PartialOrd<Self> {
/// This method returns an `Ordering` between `self` and `other`.
Expand Down Expand Up @@ -242,15 +337,57 @@ impl PartialOrd for Ordering {
/// transitively: if `T: PartialOrd<U>` and `U: PartialOrd<V>` then `U: PartialOrd<T>` and `T:
/// PartialOrd<V>`.
///
/// ## Derivable
///
/// This trait can be used with `#[derive]`. When `derive`d, it will produce a lexicographic
/// ordering based on the top-to-bottom declaration order of the struct's members.
///
/// ## How can I implement `Ord`?
///
/// PartialOrd only requires implementation of the `partial_cmp` method, with the others generated
/// from default implementations.
///
/// However it remains possible to implement the others separately for types which do not have a
/// total order. For example, for floating point numbers, `NaN < 0 == false` and `NaN >= 0 ==
/// false` (cf. IEEE 754-2008 section 5.11).
///
/// This trait can be used with `#[derive]`. When `derive`d, it will produce an ordering
/// based on the top-to-bottom declaration order of the struct's members.
/// `PartialOrd` requires your type to be `PartialEq`.
///
/// If your type is `Ord`, you can implement `partial_cmp` by using `cmp`:
///
/// ```
/// impl PartialOrd for Person {
/// fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
/// Some(self.cmp(other))
/// }
/// }
/// ```
///
/// You may also find it useful to use `partial_cmp` on your type`s fields. Here
/// is an example of `Person` types who have a floating-point `height` field that
/// is the only field to be used for sorting:
///
/// ```
/// use std::cmp::Ordering;
///
/// struct Person {
/// id: u32,
/// name: String,
/// height: f64,
/// }
///
/// impl PartialOrd for Person {
/// fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
/// self.height.partial_cmp(&other.height)
/// }
/// }
///
/// impl PartialEq for Person {
/// fn eq(&self, other: &Self) -> bool {
/// self.height == other.height
/// }
/// }
/// ```
///
/// # Examples
///
Expand Down
29 changes: 26 additions & 3 deletions src/libcore/default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,33 @@

use marker::Sized;

/// A trait for giving a type a useful default value.
/// A trait for giving a type a useful default value. For more information, see
/// [the module-level documentation][module].
Copy link
Contributor

@steveklabnik steveklabnik May 23, 2016

Choose a reason for hiding this comment

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

Same here with regards to summaries, I'd move this second sentence into its own paragraph.

Or, ideally, move that module-level documentation into the type itself. It's always been a bit tricky/scattered to tell where things should go; I'm slowly becoming of the opinion that this kind of thing is an antipattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooooh I'd love moving the module-level docs into here! I think this is where you'd likely end up after getting an error message that you needed a trait and searching in the docs for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so originally the thinking was like "docs shouldn't be repeated, they should only go in one place, what do?" But really, the module-level should provide a broad summary, the the struct itself should have all the gory details.

///
/// A struct can derive default implementations of `Default` for basic types using
/// `#[derive(Default)]`.
/// [module]: ../../std/default/index.html
///
/// ## Derivable
///
/// This trait can be used with `#[derive]` if all of the type's fields implement
/// `Default`. When `derive`d, it will use the default value for each field's type.
///
/// ## How can I implement `Default`?
///
/// Provide an implementation for the `default()` method that returns the value of
/// your type that should be the default:
///
/// ```
/// # #![allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is dead code because default() is never called, I guess?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so? I don't really understand why some existing examples have allow(dead_code) and some don't. I didn't see any compiler warnings when running make check-docs. Do we aim to have warningless code if someone opens this in the playground?

/// enum Kind {
/// A,
/// B,
/// C,
/// }
///
/// impl Default for Kind {
/// fn default() -> Kind { Kind::A }
/// }
/// ```
///
/// # Examples
///
Expand Down
6 changes: 5 additions & 1 deletion src/libcore/fmt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,11 @@ impl<'a> Display for Arguments<'a> {
///
/// [module]: ../../std/fmt/index.html
///
/// This trait can be used with `#[derive]`.
/// This trait can be used with `#[derive]` if all fields implement `Debug`. When
/// `derive`d for structs, it will use the name of the `struct`, then `{`, then a
/// comma-separated list of each field's name and `Debug` value, then `}`. For
/// `enum`s, it will use the name of the variant and, if applicable, `(`, then the
/// `Debug` values of the fields, then `)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

How stable do we consider the exact output of the format traits? /cc @aturon

///
/// # Examples
///
Expand Down
28 changes: 27 additions & 1 deletion src/libcore/hash/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,33 @@ mod sip;
/// In other words, if two keys are equal, their hashes should also be equal.
/// `HashMap` and `HashSet` both rely on this behavior.
///
/// This trait can be used with `#[derive]`.
/// ## Derivable
///
/// This trait can be used with `#[derive]` if all fields implement `Hash`.
/// When `derive`d, the resulting hash will be the combination of the values
/// from calling `.hash()` on each field.
///
/// ## How can I implement `Hash`?
///
/// If you need more control over how a value is hashed, you need to implement
/// the trait `Hash`:
Copy link
Contributor

Choose a reason for hiding this comment

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

extremely minor nit: we almost always say "the Hash trait" rather than "the trait Hash"

///
/// ```
/// use std::hash::{Hash, Hasher};
///
/// struct Person {
/// id: u32,
/// name: String,
/// phone: u64,
/// }
///
/// impl Hash for Person {
/// fn hash<H: Hasher>(&self, state: &mut H) {
/// self.id.hash(state);
/// self.phone.hash(state);
/// }
/// }
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub trait Hash {
/// Feeds this value into the state given, updating the hasher as necessary.
Expand Down
39 changes: 20 additions & 19 deletions src/libcore/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,26 @@ pub trait Unsize<T: ?Sized> {
/// the trait `Copy` may not be implemented for this type; field `points` does not implement `Copy`
/// ```
///
/// ## When can my type _not_ be `Copy`?
///
/// Some types can't be copied safely. For example, copying `&mut T` would create an aliased
/// mutable reference, and copying `String` would result in two attempts to free the same buffer.
///
/// Generalizing the latter case, any type implementing `Drop` can't be `Copy`, because it's
/// managing some resource besides its own `size_of::<T>()` bytes.
///
/// ## When should my type be `Copy`?
///
/// Generally speaking, if your type _can_ implement `Copy`, it should. There's one important thing
/// to consider though: if you think your type may _not_ be able to implement `Copy` in the future,
/// then it might be prudent to not implement `Copy`. This is because removing `Copy` is a breaking
/// change: that second example would fail to compile if we made `Foo` non-`Copy`.
///
/// ## Derivable
///
/// This trait can be used with `#[derive]` if all of its components implement `Copy` and the type
/// implements `Clone`. The implementation will copy the bytes of each field using `memcpy`.
///
/// ## How can I implement `Copy`?
///
/// There are two ways to implement `Copy` on your type:
Expand All @@ -155,25 +175,6 @@ pub trait Unsize<T: ?Sized> {
///
/// There is a small difference between the two: the `derive` strategy will also place a `Copy`
/// bound on type parameters, which isn't always desired.
///
/// ## When can my type _not_ be `Copy`?
///
/// Some types can't be copied safely. For example, copying `&mut T` would create an aliased
/// mutable reference, and copying `String` would result in two attempts to free the same buffer.
///
/// Generalizing the latter case, any type implementing `Drop` can't be `Copy`, because it's
/// managing some resource besides its own `size_of::<T>()` bytes.
///
/// ## When should my type be `Copy`?
///
/// Generally speaking, if your type _can_ implement `Copy`, it should. There's one important thing
/// to consider though: if you think your type may _not_ be able to implement `Copy` in the future,
/// then it might be prudent to not implement `Copy`. This is because removing `Copy` is a breaking
/// change: that second example would fail to compile if we made `Foo` non-`Copy`.
///
/// # Derivable
///
/// This trait can be used with `#[derive]`.
#[stable(feature = "rust1", since = "1.0.0")]
#[lang = "copy"]
pub trait Copy : Clone {
Expand Down