Fix wording around sort guarantees #38961

Open
wants to merge 5 commits into
from

Projects

None yet

3 participants

@steveklabnik
Contributor
@stjepang

You changed the documentation of sort(), but forgot about sort_by and sort_by_key :)

The current changes will resolve the discussed normativity issue, which is good. However, I'd like to push for higher quality documentation while we're at it.

I left a few suggestions on how to do that. We don't need long boring walls of text. Just a few points on the key characteristics of this particular implementation.

src/libcollections/slice.rs
@@ -1041,8 +1041,11 @@ impl<T> [T] {
/// This is equivalent to `self.sort_by(|a, b| a.cmp(b))`.
///
- /// This sort is stable and `O(n log n)` worst-case, but allocates
- /// temporary storage half the size of `self`.
+ /// This sort is stable and `O(n log n)` worst-case.
@stjepang
stjepang Jan 10, 2017 Contributor

Let's explain what "stable" means, since it can be a confusing term for people unfamiliar with it.
Java's docs have a nice concise explanation: "This sort is guaranteed to be stable: equal elements will not be reordered as a result of the sort."

src/libcollections/slice.rs
- /// temporary storage half the size of `self`.
+ /// This sort is stable and `O(n log n)` worst-case.
+ ///
+ /// # Current Implementation
@stjepang
stjepang Jan 10, 2017 edited Contributor

Just nitpicking here: lowercase "implementation" would be more consistent with other docs in libstd.

src/libcollections/slice.rs
+ ///
+ /// # Current Implementation
+ ///
+ /// The current implementation allocates temporary storage half the size of `self`.
@stjepang
stjepang Jan 10, 2017 edited Contributor

Now that we have a section about the current implementation, this would be a great chance to expand a little bit. For inspiration, see Java's docs starting from "Implementation note".

I would like to see several things pointed out:

  1. That this sort is an adaptation of Tim Peters' timsort with this link.
  2. That this is an iterative sort.
  3. That this is an adaptive sort. Again, Java has a nice explanation: "It is well-suited to merging two or more sorted arrays: simply concatenate the arrays and sort the resulting array."
@stjepang
Contributor

One more thing I noticed...

sort_by has this explanation: Sorts the slice, in place, using compare to compare elements.
I'd like to see "in place" gone because it sounds like sort_by does not allocate, which is incorrect.

sort_by_key has this explanation: Sorts the slice, in place, using f to extract a key by which to order the sort by.
We again have "in place" but this time it at least makes sense - it's trying to say that sort_by_key does not do decorate-sort-undecorate.
However, I'm not too happy with "in place" here either and would prefer different wording.

@steveklabnik
Contributor

@brson @alexcrichton are we going to backport this? If so, I can wrap it up right now, but if not, I'd rather not drop everything to do so. know you wanted to get beta done by today

@alexcrichton
Member

I'd be ambivalent about a backport. I wouldn't advocate for it but I also wouldn't say no.

@alexcrichton
Member

@steveklabnik looks like there's a whitespace error here, but other than that r=me

@alexcrichton alexcrichton self-assigned this Jan 19, 2017
@alexcrichton alexcrichton added the T-libs label Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment