Doc total order requirement of sort(_unstable)_by #53918

Havvy · 2018-09-03T05:29:37Z

I took the definition of what a total order is from the Ord trait
docs. I specifically put "elements of the slice" because if you
have a slice of f64s, but know none are NaN, then sorting by
partial ord is total in this case. I'm not sure if I should give
such an example in the docs or not.

r? @GuillaumeGomez

GuillaumeGomez · 2018-09-03T07:52:10Z

Examples are always a big plus so please add them. :)

Havvy · 2018-09-03T09:00:18Z

Alright. I'll write them tomorrow. Sleep beckons.

Havvy · 2018-09-10T05:12:59Z

Tomorrow turned into a week. Whoops.

GuillaumeGomez · 2018-09-10T08:28:01Z

src/liballoc/slice.rs

@@ -218,6 +218,15 @@ impl<T> [T] {
    /// * 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 >.
    ///
+    /// For example, while `f64` doesn't implement `Ord` because `NaN != NaN`, we can use


Please add urls to types.

GuillaumeGomez · 2018-09-10T08:28:10Z

src/libcore/slice/mod.rs

@@ -1350,6 +1350,15 @@ impl<T> [T] {
    /// * 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 >.
    ///
+    /// For example, while `f64` doesn't implement `Ord` because `NaN != NaN`, we can use


Havvy · 2018-09-25T21:18:26Z

ping @GuillaumeGomez

GuillaumeGomez · 2018-09-26T23:14:10Z

src/liballoc/slice.rs

+    /// * transitive, a < b and b < c implies a < c. The same must hold for both == and >.
+    ///
+    /// For example, while [`f64`] doesn't implement [`Ord`] because `NaN != NaN`, we can use
+    /// `partial_cmp` as our sort function when we know the slice doesn't contain a `NaN`.


Can you link to partial_cmp please?

GuillaumeGomez · 2018-09-26T23:14:31Z

src/libcore/slice/mod.rs

+    /// * transitive, a < b and b < c implies a < c. The same must hold for both == and >.
+    ///
+    /// For example, while [`f64`] doesn't implement [`Ord`] because `NaN != NaN`, we can use
+    /// `partial_cmp` as our sort function when we know the slice doesn't contain a `NaN`.


GuillaumeGomez · 2018-09-26T23:15:53Z

Once the links added, it's good for me.

cc @rust-lang/docs (in case someone would like to double check).

QuietMisdreavus · 2018-10-04T22:06:41Z

+1. Can you add the links @GuillaumeGomez requested?

I took the definition of what a total order is from the Ord trait docs. I specifically put "elements of the slice" because if you have a slice of f64s, but know none are NaN, then sorting by partial ord is total in this case. I'm not sure if I should give such an example in the docs or not.

Havvy · 2018-10-07T06:38:09Z

Links added and rebased against master.

rust-highfive · 2018-10-07T08:15:28Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:53:06] ................................................................................................i... 2200/4563
[00:53:10] .................................................................................................... 2300/4563
[00:53:14] .................................................................................................... 2400/4563
[00:53:18] .................................................................................................... 2500/4563
[00:53:21] ........iiiiiiiii................................................................................... 2600/4563
[00:53:27] .................................................................................................... 2800/4563
[00:53:31] .................................................................................................... 2900/4563
[00:53:34] ............................i....................................................................... 3000/4563
[00:53:37] ........................................................................................i.i..ii..... 3100/4563
---
[01:31:53] travis_fold:end:stage0-linkchecker

[01:31:53] travis_time:end:stage0-linkchecker:start=1538899941792582952,finish=1538899944042187862,duration=2249604910

[01:33:50] std/vec/struct.Vec.html:1350: broken link - std/std/cmp/trait.PartialOrd.html
[01:33:50] std/vec/struct.Vec.html:1700: broken link - std/std/cmp/trait.PartialOrd.html
[01:34:45] thread 'main' panicked at 'found some broken links', tools/linkchecker/main.rs:49:9
[01:34:45] 
[01:34:45] 
[01:34:45] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/linkchecker" "/checkout/obj/build/x86_64-unknown-linux-gnu/doc"
[01:34:45] expected success, got: exit code: 101
[01:34:45] expected success, got: exit code: 101
[01:34:45] 
[01:34:45] 
[01:34:45] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:34:45] Build completed unsuccessfully in 0:46:26
[01:34:45] Makefile:58: recipe for target 'check' failed
[01:34:45] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2bf6801e
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:183b6354:start=1538900124460003233,finish=1538900124591751604,duration=131748371
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0d48c286
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0a02c396
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Havvy · 2018-10-07T09:11:59Z

I'm confused. Those links worked locally. I'll just add the "../" to make linkchecker happy though, and perhaps I misread what happened on my end?

rust-highfive · 2018-10-07T10:40:28Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:47:37] ................................................................................................i... 2200/4563
[00:47:41] .................................................................................................... 2300/4563
[00:47:44] .................................................................................................... 2400/4563
[00:47:48] .................................................................................................... 2500/4563
[00:47:51] ........iiiiiiiii................................................................................... 2600/4563
[00:47:57] .................................................................................................... 2800/4563
[00:48:01] .................................................................................................... 2900/4563
[00:48:03] ............................i....................................................................... 3000/4563
[00:48:06] ........................................................................................i.i..ii..... 3100/4563
---
[01:22:53] travis_fold:end:stage0-linkchecker

[01:22:53] travis_time:end:stage0-linkchecker:start=1538908626887235069,finish=1538908629074560159,duration=2187325090

[01:22:53] std/primitive.slice.html:633: broken link - /checkout/obj/build/x86_64-unknown-linux-gnu/std/cmp/trait.PartialOrd.html
[01:22:53] std/primitive.slice.html:983: broken link - /checkout/obj/build/x86_64-unknown-linux-gnu/std/cmp/trait.PartialOrd.html
[01:26:02] thread 'main' panicked at 'found some broken links', tools/linkchecker/main.rs:49:9
[01:26:02] 
[01:26:02] 
[01:26:02] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/linkchecker" "/checkout/obj/build/x86_64-unknown-linux-gnu/doc"
[01:26:02] expected success, got: exit code: 101
[01:26:02] expected success, got: exit code: 101
[01:26:02] 
[01:26:02] 
[01:26:02] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:26:02] Build completed unsuccessfully in 0:43:05
[01:26:02] make: *** [check] Error 1
[01:26:02] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:03952c38
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

GuillaumeGomez · 2018-10-07T12:33:49Z

src/liballoc/slice.rs

@@ -252,6 +252,8 @@ impl<T> [T] {
    /// v.sort_by(|a, b| b.cmp(a));
    /// assert!(v == [5, 4, 3, 2, 1]);
    /// ```
+    ///
+    /// [`partial_cmp`]: ../../std/cmp/trait.PartialOrd.html#tymethod.partial_cmp


Just remove this.

That causes the rustdoc output to say:

222 | /// [`partial_cmp`] as our sort function when we know the slice doesn't contain a `NaN`. | ^^^^^^^^^^^^^ cannot be resolved, ignoring

GuillaumeGomez · 2018-10-07T12:34:00Z

src/libcore/slice/mod.rs

@@ -1378,6 +1378,7 @@ impl<T> [T] {
    /// assert!(v == [5, 4, 3, 2, 1]);
    /// ```
    ///
+    /// [`partial_cmp`]: ../../std/cmp/trait.PartialOrd.html#tymethod.partial_cmp


And remove this one too.

QuietMisdreavus · 2018-10-09T14:05:17Z

That partial_ord cannot be linked to, either from directly linking to the file or via intra-doc links.

The core issue with linking directly to the file is that these docs appear in two places: directly on slices (std/primitive.slice.html) and on Vec when documented in std (std/vec/struct.Vec.html - they're not in alloc because the slice docs aren't available there). You can't use different amounts of ../ to appease both places.
You could try to use intra-doc links like @GuillaumeGomez suggests, but you need to link to the libcore version of PartialOrd, which may cause the eventual docs to link to core instead of std like we prefer. If you want to try it, link to the method via the full declaration of the PartialOrd trait - so core::cmp::PartialOrd::partial_cmp (cmp::PartialOrd::partial_cmp for the location in libcore) - then see whether the docs in std link to core or std. I have a feeling they'll link to core, but i haven't looked closely enough at the relevant code to know for sure.

TimNN · 2018-10-30T17:33:11Z

ping from triage @Havvy: it looks like some changes have been requested to this PR.

TimNN · 2018-11-06T18:27:57Z

Ping from triage @Havvy: What are your plans for this PR?

Havvy · 2018-11-07T22:25:30Z

I removed the commits for linkifying partial_cmp since I don't think it's possible given the link has to work in two places and cannot be done with intra-linking.

TimNN · 2018-11-13T18:14:19Z

Ping from triage @GuillaumeGomez: it looks like this PR is ready for another review.

TimNN · 2018-11-20T20:20:44Z

Ping from triage @GuillaumeGomez / @rust-lang/docs: This PR requires your review.

GuillaumeGomez · 2018-11-21T09:19:20Z

Then it's all good as is. Thanks!

@bors: r+

bors · 2018-11-21T09:19:21Z

📌 Commit 99bed21 has been approved by GuillaumeGomez

bors · 2018-11-22T06:50:26Z

⌛ Testing commit 99bed21 with merge f3adec6...

@GuillaumeGomez

Doc total order requirement of sort(_unstable)_by I took the definition of what a total order is from the Ord trait docs. I specifically put "elements of the slice" because if you have a slice of f64s, but know none are NaN, then sorting by partial ord is total in this case. I'm not sure if I should give such an example in the docs or not. r? @GuillaumeGomez

bors · 2018-11-22T10:04:35Z

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

rust-highfive assigned GuillaumeGomez Sep 3, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 3, 2018

Havvy force-pushed the doc-sort-by branch from 2eaff73 to 0c7b419 Compare September 4, 2018 06:12

GuillaumeGomez requested changes Sep 10, 2018

View reviewed changes

Havvy force-pushed the doc-sort-by branch from c751086 to 2b56f08 Compare September 11, 2018 02:28

GuillaumeGomez reviewed Sep 26, 2018

View reviewed changes

Havvy added 4 commits October 5, 2018 17:41

Example of total ord of elements for sort_by

e36bbc8

Slice total example: Move closer to total defn

b911dba

Linkify types in docs

99bed21

Havvy force-pushed the doc-sort-by branch from 2b56f08 to 2504cd4 Compare October 7, 2018 06:37

Havvy force-pushed the doc-sort-by branch from 2504cd4 to 8f79c0c Compare October 7, 2018 09:10

GuillaumeGomez reviewed Oct 7, 2018

View reviewed changes

TimNN added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2018

Havvy force-pushed the doc-sort-by branch from 8f79c0c to 99bed21 Compare November 7, 2018 22:24

TimNN added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 13, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2018

bors merged commit 99bed21 into rust-lang:master Nov 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Doc total order requirement of sort(_unstable)_by #53918

Doc total order requirement of sort(_unstable)_by #53918

Havvy commented Sep 3, 2018

GuillaumeGomez commented Sep 3, 2018

Havvy commented Sep 3, 2018

Havvy commented Sep 10, 2018

GuillaumeGomez Sep 10, 2018

GuillaumeGomez Sep 10, 2018

Havvy commented Sep 25, 2018

GuillaumeGomez Sep 26, 2018

GuillaumeGomez Sep 26, 2018

GuillaumeGomez commented Sep 26, 2018

QuietMisdreavus commented Oct 4, 2018

Havvy commented Oct 7, 2018

rust-highfive commented Oct 7, 2018

Havvy commented Oct 7, 2018

rust-highfive commented Oct 7, 2018

GuillaumeGomez Oct 7, 2018

Havvy Oct 8, 2018

GuillaumeGomez Oct 7, 2018

QuietMisdreavus commented Oct 9, 2018

TimNN commented Oct 30, 2018

TimNN commented Nov 6, 2018

Havvy commented Nov 7, 2018

TimNN commented Nov 13, 2018

TimNN commented Nov 20, 2018

GuillaumeGomez commented Nov 21, 2018

bors commented Nov 21, 2018

bors commented Nov 22, 2018

bors commented Nov 22, 2018

Doc total order requirement of sort(_unstable)_by #53918

Doc total order requirement of sort(_unstable)_by #53918

Conversation

Havvy commented Sep 3, 2018

GuillaumeGomez commented Sep 3, 2018

Havvy commented Sep 3, 2018

Havvy commented Sep 10, 2018

GuillaumeGomez Sep 10, 2018

Choose a reason for hiding this comment

GuillaumeGomez Sep 10, 2018

Choose a reason for hiding this comment

Havvy commented Sep 25, 2018

GuillaumeGomez Sep 26, 2018

Choose a reason for hiding this comment

GuillaumeGomez Sep 26, 2018

Choose a reason for hiding this comment

GuillaumeGomez commented Sep 26, 2018

QuietMisdreavus commented Oct 4, 2018

Havvy commented Oct 7, 2018

rust-highfive commented Oct 7, 2018

Havvy commented Oct 7, 2018

rust-highfive commented Oct 7, 2018

GuillaumeGomez Oct 7, 2018

Choose a reason for hiding this comment

Havvy Oct 8, 2018

Choose a reason for hiding this comment

GuillaumeGomez Oct 7, 2018

Choose a reason for hiding this comment

QuietMisdreavus commented Oct 9, 2018

TimNN commented Oct 30, 2018

TimNN commented Nov 6, 2018

Havvy commented Nov 7, 2018

TimNN commented Nov 13, 2018

TimNN commented Nov 20, 2018

GuillaumeGomez commented Nov 21, 2018

bors commented Nov 21, 2018

bors commented Nov 22, 2018

bors commented Nov 22, 2018