Improve std::ops docs #43724

chordowl · 2017-08-07T22:18:14Z

Fixes #29365. (This fixes all but one point from @steveklabnik's list, but that point was referring to examples of implementing range traits, but there are no range traits in std::ops.)

The main changes are quite a bit of copyediting, adding more "real" examples for some of the traits, incorporating some guidance from the API docs, more linking (cross-docs and to the book & reference), cleaning up examples, moving things around, and so on. Refer to the commit messages for more details.

Note: I decided to link to the second edition of the book since I think it's more appropriate now for the sections I linked, if this is not okay, please say so!

Part of #29365. * Replaced examples for Mul-/Div-/RemAssign with more illustrative ones * Made summary senteces for the trait methods use third person singular * Moved some explanations from Examples section to main explanation * Switched around argument order for the vector-scalar multiplication example such that the vector is on the left side (as it would be expected if one were to switch from `*` to `*=`) * Replaced mostly redundant example introductions with headings in traits with more than one example (where it made sense) * Cleaned up some examples to derive `PartialEq` instead of implementing it manually when that wasn't needed * Removed explicit `fn main()`s in examples where they weren't necessary * Rephrased some things * Added some missing periods * Fixed some formatting/punctuation in examples

Part of #29365. * Added "real" examples for `BitOrAssign`, `BitXorAssign`, `ShlAssign`, and `ShrAssign` * Rewrote method summary senteces to be in 3rd person singular * Rephrased example introductions to be less redundant ("in this example" etc.) and to not use "trivial" * Removed superfluous explicit `fn main()`s in examples * Added some missing periods

Part of #29365. * Expanded the explanaition sections, adapting some parts from the book, the reference, as well as the API guidelines. As such, the docs now explicitly state that `Deref` and `DerefMut` should only be implemented for smart pointers and that they should not fail. Additionally, there is now a short primer on `Deref` coercion. * Added links to `DerefMut` from `Deref` and vice versa * Added links to relevant reference sections * Removed "stuttering" in summary sentences * Changed summary sentences of `Deref::deref` and `Deref::deref_mut` to be in 3rd person singular * Removed explicit uses of `fn main()` in the examples

Part of #29365. * Removed "stuttering" in summary sentence. * Copy-edited the explanaition sections * Added sub-headings in Examples section to aid linking * Actually implement `Drop` in the `PrintOnDrop` exampl * Add link to Drop chapter in TRPL * Changed `drop` summary sentence to be in 3rd person singular * Added missing link to `panic!`

Part of #29365. * Shortened summary sentences, removing "stuttering" * Small copyediting * Changed method summary sentences to be in 3rd person singular * Removed extraneous explicit `fn main()` in example for `IndexMut`

Part of #29365. * Moved explanations out of Examples section and expanded on them. * Made the super-/subtrait relationships more explicit. * Added links to the other traits, TRPL and the nomicon where appropriate * Changed method summaries to be in 3rd person singular * General copyediting

Part of #29365. * Strenghtened summary/explanation split, making phrasings more parallel * Added links throughout * Fixed some example formatting & removed extraneous `fn main()`s (or hid then when needed because of `#![features]`. * Emphasized note on `RangeFrom`'s `Iterator` implementation * Added summary sentences to (unstable) `contains` methods

Part of #29365. * Added paragraph adapted from API guidelines that operator implementations should be unsurprising * Modified Point example to be more clear when just reading it

kennytm · 2017-08-08T08:04:24Z

src/libcore/ops/function.rs

+/// this can refer to [the relevant section in *The Rustonomicon*][nomicon].
+///
+/// [book]: ../../book/second-edition/ch13-01-closures.html
+/// [`Fn`]: trait.Fnhtml


Typo. This is causing CI failure.

[01:08:12] std/ops/trait.FnMut.html:59: broken link - std/ops/trait.Fnhtml [01:08:12] std/ops/trait.FnMut.html:62: broken link - std/ops/trait.Fnhtml [01:08:12] std/ops/trait.FnMut.html:64: broken link - std/ops/trait.Fnhtml [01:08:12] std/ops/trait.FnMut.html:65: broken link - std/ops/trait.Fnhtml [01:08:18] core/ops/trait.FnMut.html:59: broken link - core/ops/trait.Fnhtml [01:08:18] core/ops/trait.FnMut.html:62: broken link - core/ops/trait.Fnhtml [01:08:18] core/ops/trait.FnMut.html:64: broken link - core/ops/trait.Fnhtml [01:08:18] core/ops/trait.FnMut.html:65: broken link - core/ops/trait.Fnhtml [01:08:22] thread 'main' panicked at 'found some broken links', /checkout/src/tools/linkchecker/main.rs:49:8

arielb1 · 2017-08-08T11:04:53Z

r? @steveklabnik I think

chordowl · 2017-08-08T12:35:43Z

Fixed link typo as well as two erroneous attempted "links".

steveklabnik · 2017-08-08T16:51:13Z

Note: I decided to link to the second edition of the book since I think it's more appropriate now for the sections I linked, if this is not okay, please say so!

It very much is!

steveklabnik

I think this looks great! Thanks a ton! This has a lot of stuff, so I'd like another docs person to review too.

steveklabnik · 2017-08-08T16:59:03Z

src/libcore/ops/arith.rs

@@ -230,17 +200,19 @@ sub_impl! { usize u8 u16 u32 u64 u128 isize i8 i16 i32 i64 i128 f32 f64 }

 /// The multiplication operator `*`.
 ///
+/// Note that `RHS = Self` by default, but this is not mandatory.


I might say "RHS is Self" instead, because the = is real syntax and I find this wording slightly confusing because of it.

i might also be a weirdo

I think that's a good point, I'll change it (after the other review)

steveklabnik · 2017-08-08T17:01:40Z

src/libcore/ops/function.rs

@@ -10,24 +10,37 @@

 /// A version of the call operator that takes an immutable receiver.
 ///
-/// # Examples
+/// Closures only taking immutable references to captured variables
+/// automatically implement this trait, which allows them to be invoked.


the "which allows them to be invoked" feels awkward to me, but I'm not sure I have good suggestions.

You may also want to mention that function pointers (and closures with no captures at all) also implement Fn.

steveklabnik · 2017-08-08T17:02:20Z

src/libcore/ops/range.rs

-    ///     assert!( ! (3..3).contains(3));
-    ///     assert!( ! (3..2).contains(3));
-    /// }
+    /// # fn main() {


i'm not sure why fn main is here, probably should just drop it

it's here because I was confused about how the fn main() insertion interacts with features in doctests 😅

Since feature flags need to be at the crate root, you need a fn main so rustdoc doesn't wrap the doctest with the regular boilerplate.

...on the other hand, one of the RangeToInclusive examples doesn't have the wrapper? It might be worth checking to see whether taking the "fn main" stuff out still works.

QuietMisdreavus

some flow and consistency nits. otherwise really good, thanks!

QuietMisdreavus · 2017-08-09T21:28:56Z

src/libcore/ops/deref.rs

+///
+/// # More on `Deref` coercion
+///
+/// If `T` implements `MutDeref<Target = U>`, and `x` is a value of type `T`,


MutDeref?

QuietMisdreavus · 2017-08-09T21:36:06Z

src/libcore/ops/function.rs

@@ -10,24 +10,37 @@

 /// A version of the call operator that takes an immutable receiver.
 ///
-/// # Examples
+/// Closures only taking immutable references to captured variables
+/// automatically implement this trait, which allows them to be invoked.


You may also want to mention that function pointers (and closures with no captures at all) also implement Fn.

QuietMisdreavus · 2017-08-09T21:39:26Z

src/libcore/ops/function.rs

+/// Closures that might mutably reference captured variables automatically
+/// implement this trait, which allows them to be invoked. For immutably
+/// referenced captures, see [`Fn`], and for consuming the captures, see
+/// [`FnOnce`].


Ditto for no captures at all getting Fn.

QuietMisdreavus · 2017-08-09T21:43:15Z

src/libcore/ops/function.rs

+/// Closures that might take ownership of captured variables automatically
+/// implement this trait, which allows them to be invoked. For immutably
+/// referenced captures, see [`Fn`], and for mutably referenced captures,
+/// see [`FnMut`].


"Taking ownership" isn't exactly right. "move" closures take ownership of their captures, but they can still be Fn or FnMut if they don't consume them. Those captures just become part of its state. FnOnce closures allow you to do something like into() or into_iter() or something else that consumes a value.

QuietMisdreavus · 2017-08-09T21:46:07Z

src/libcore/ops/function.rs

+///
+/// You can use the `Fn` traits when you want to accept a closure as a
+/// parameter. Since both [`FnMut`] and [`FnOnce`] are supertraits of `Fn`, any
+/// instance of `Fn` can be used where a [`FnMut`] or [`FnOnce`] is expected.


I'd like to have a mention that &T references to closures that are Fn also implement Fn, so you can declare a closure separately, pass it by-reference to a place that needs it, and still have it around to pass somewhere else. &T where T: Fn implements all the closure traits, just like the regular T itself.

QuietMisdreavus · 2017-08-09T21:52:56Z

src/libcore/ops/range.rs

 ///
-/// Its primary use case is slicing index. It cannot serve as an iterator
-/// because it doesn't have a starting point.
+/// `RangeFull` is primarily used as a [slicing index], it's shorthand is `..`.


s/it's/its

(And i'd personally trade the comma for a semicolon or a period >_>)

QuietMisdreavus · 2017-08-09T21:55:57Z

src/libcore/ops/range.rs

-    ///     assert!( ! (3..3).contains(3));
-    ///     assert!( ! (3..2).contains(3));
-    /// }
+    /// # fn main() {


Since feature flags need to be at the crate root, you need a fn main so rustdoc doesn't wrap the doctest with the regular boilerplate.

...on the other hand, one of the RangeToInclusive examples doesn't have the wrapper? It might be worth checking to see whether taking the "fn main" stuff out still works.

QuietMisdreavus · 2017-08-09T21:57:37Z

src/libcore/ops/range.rs

-/// It does not have an `IntoIterator` implementation, so you can't use it in a
-/// `for` loop directly. This won't compile:
+/// It does not have an [`IntoIterator`] implementation, so you can't use it in
+/// a `for` loop directly. This won't compile:
 ///
 /// ```compile_fail,E0277
 /// for i in ..5 {
 ///     // ...
 /// }
 /// ```


Would be nice to have some comment inside the code block so people don't try to run it themselves. Just copying the error message from E0277 into a comment would suffice.

QuietMisdreavus · 2017-08-09T22:02:38Z

src/libcore/ops/range.rs

@@ -320,15 +335,22 @@ impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
 /// }
 /// ```


Ditto my comment on the similar block in RangeTo, about putting the compile error into the sample.

QuietMisdreavus · 2017-08-09T22:09:21Z

src/libcore/ops/arith.rs

@@ -526,19 +482,19 @@ div_impl_float! { f32 f64 }
 /// }
 ///
 /// // If we were to divide &[0, 1, 2, 3, 4, 5, 6, 7] into slices of size 3,
-/// // the remainder would be &[6, 7]
+/// // the remainder would be &[6, 7].
 /// assert_eq!(SplitSlice { slice: &[0, 1, 2, 3, 4, 5, 6, 7] } % 3,
 ///            SplitSlice { slice: &[6, 7] });
 /// ```
 #[lang = "rem"]
 #[stable(feature = "rust1", since = "1.0.0")]
 #[rustc_on_unimplemented = "no implementation for `{Self} % {RHS}`"]
 pub trait Rem<RHS=Self> {


Would be cool to add the "Note that RHS is Self" line here too, for consistency.

(Pretend i wrote this on all the op-assign traits, too >_>)

chordowl · 2017-08-10T21:49:56Z

Reviews should all be addressed in the latest commit! Please look at Fn etc. again, I mostly rewrote the text again with the goal of being more accurate and (hopefully) more clear. (It also turned out that I could remove all the fn main()s.)

QuietMisdreavus

The new Fn* descriptions are good! Just a couple more nits >_>;;

QuietMisdreavus · 2017-08-10T22:12:20Z

src/libcore/ops/function.rs

+///
+/// `Fn` is implemented automatically by closures which only take immutable
+/// references to captured variables or don't capture anything at all, as well
+/// as (safe) [function pointers][] (with some caveats, see their documentation


probably don't need the [] there?

I checked, and I do need them (because of hoedown, I wouldn't in CommonMark 😒)

Oh right, it's followed by the parenthetical -_-

QuietMisdreavus · 2017-08-10T22:14:06Z

src/libcore/ops/function.rs

+/// references to captured variables, as well as all types that implement
+/// [`Fn`], e.g. (safe) [function pointers][] (since `FnMut` is a supertrait of
+/// [`Fn`]). Additionally, for any type `F` that implements `FnMut`, both `&F`
+/// and `&mut F` implement `FnMut`, too.


(actually &F where F: FnMut doesn't impl FnMut - it needs to mutate its receiver >_>)

QuietMisdreavus · 2017-08-10T22:17:05Z

src/libcore/ops/function.rs

+///
+/// Use `FnMut` as a bound when you want to accept a parameter of function-like
+/// type and need to call it repeatedly, without requiring it not to mutate
+/// state. If you require the parameter to not mutate state, use [`Fn`] as a


I'd prefer to say "while allowing it to mutate state", just to get around the double-negative there

Ditto for "If you don't want the parameter to mutate state,"

QuietMisdreavus · 2017-08-10T22:19:53Z

src/libcore/ops/function.rs

-/// A version of the call operator that takes a by-value receiver.
+/// The version of the call operator that takes a by-value receiver.
+///
+/// Instances `FnOnce` can be called, but might not be callable multiple times.


Would prefer something like "Instances of FnOnce can be called once, and only once." Just take out the ambiguity when you're talking about FnOnce by itself, then bring in the other Fn* traits below.

The reason for phrasing it this way is that any instance of e,g, Fn is also an instance of FnOnce, so an instance of Fn, while being an instance of FnOnce, can be called multiple times. That's why I added the second sentence about when the only known thing is FnOnce.

Reading over this again, i think it'll work. However, "Instances [of] FnOnce", missing an "of".

r=me once that's done!

QuietMisdreavus · 2017-08-10T22:20:30Z

src/libcore/ops/function.rs

-/// parameter. Since both [`Fn`] and [`FnMut`] are subtraits of `FnOnce`, any
-/// instance of [`Fn`] or [`FnMut`] can be used where a `FnOnce` is expected.
+/// Use `FnMut` as a bound when you want to accept a parameter of function-like
+/// type and only need to call it once. If you need to call the parameter


s/FnMut/FnOnce

chordowl · 2017-08-11T22:00:21Z

@QuietMisdreavus I think everything should be addressed now!

* fixed link typos and copy-paster errors * rewrote Fn* explanations * `RHS = Self` -> `RHS` is `Self` (added that to all applicable places as well) * fixed up some links * s/MutDeref/DerefMut * removed remaining superfluous `fn main()`s * fixed some minor phrasings and factual errors and inaccuracies std::ops docs: Fix phrasing and factual errors/inaccuracies

QuietMisdreavus · 2017-08-12T19:06:21Z

Looks good! Thanks for working on this!

@bors r+ rollup

bors · 2017-08-12T19:06:22Z

📌 Commit 6bdba82 has been approved by QuietMisdreavus

bors · 2017-08-12T19:40:55Z

⌛ Testing commit 6bdba82 with merge f774bce...

@steveklabnik

Improve std::ops docs Fixes #29365. (This fixes all but one point from @steveklabnik's list, but that point was referring to examples of implementing range traits, but there are no range traits in std::ops.) The main changes are quite a bit of copyediting, adding more "real" examples for some of the traits, incorporating some guidance from the API docs, more linking (cross-docs and to the book & reference), cleaning up examples, moving things around, and so on. Refer to the commit messages for more details. Note: I decided to link to the second edition of the book since I think it's more appropriate now for the sections I linked, if this is not okay, please say so!

bors · 2017-08-12T22:09:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing f774bce to master...

lukaramu added 8 commits August 7, 2017 23:10

Revise Index and IndexMut docs.

ffa327b

Part of #29365. * Shortened summary sentences, removing "stuttering" * Small copyediting * Changed method summary sentences to be in 3rd person singular * Removed extraneous explicit `fn main()` in example for `IndexMut`

Added to core::ops module docs

99e44d8

Part of #29365. * Added paragraph adapted from API guidelines that operator implementations should be unsurprising * Modified Point example to be more clear when just reading it

kennytm reviewed Aug 8, 2017

View reviewed changes

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

arielb1 assigned steveklabnik Aug 8, 2017

steveklabnik reviewed Aug 8, 2017

View reviewed changes

QuietMisdreavus reviewed Aug 9, 2017

View reviewed changes

QuietMisdreavus reviewed Aug 10, 2017

View reviewed changes

bors merged commit 6bdba82 into rust-lang:master Aug 12, 2017

bors mentioned this pull request Aug 12, 2017

Point "deref coercions" links to new book #43631

Merged

chordowl deleted the std-ops-docs branch August 12, 2017 22:28

		@@ -320,15 +335,22 @@ impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
		/// }
		/// ```

Improve std::ops docs #43724

Improve std::ops docs #43724

Conversation

chordowl commented Aug 7, 2017

Choose a reason for hiding this comment

arielb1 commented Aug 8, 2017

chordowl commented Aug 8, 2017

steveklabnik commented Aug 8, 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

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

Choose a reason for hiding this comment

QuietMisdreavus left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chordowl commented Aug 10, 2017

QuietMisdreavus left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chordowl commented Aug 11, 2017

QuietMisdreavus commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017