Skip to content

Improve core::cmp docs #21990

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.

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

Merged
merged 1 commit into from
Feb 18, 2015
Merged

Improve core::cmp docs #21990

merged 1 commit into from
Feb 18, 2015

Conversation

steveklabnik
Copy link
Member

Fix up, add examples, make them all the same.

@rust-highfive
Copy link
Contributor

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

///
/// - reflexive: `a == a`;
/// - symmetric: `a == b` implies `b == a`; and
/// - transitive: `a == b` and `b == c` implies `a == c`.
///
/// This property cannot be checked by the compiler, and so `Eq` implies `PartialEq`, and has no
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 s/and so/therefore/ would sound better here.

@steveklabnik
Copy link
Member Author

Feedback addressed.

@@ -8,35 +8,33 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! Defines the `PartialOrd` and `PartialEq` comparison traits.
//! Traits for ordering and comparison.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe s/Traits/Functionality/ since there's things other than traits? (I could just be overly nit-picky)

@steveklabnik steveklabnik force-pushed the doc_core_cmp branch 2 times, most recently from bc812af to c378c07 Compare February 11, 2015 03:29
/// assert_eq!(Ordering::Greater.reverse(), Ordering::Less);
/// ```
///
/// This method is often used to reverse a comparison:
Copy link
Member

Choose a reason for hiding this comment

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

s/is often/can be/

/// use std::cmp;
///
/// assert_eq!(Some(1), cmp::partial_min(1, 2));
/// assert_eq!(Some(2), cmp::partial_min(2, 2));
Copy link
Member

Choose a reason for hiding this comment

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

Similarly these could demonstrate returning None, also using NaN.

(Totally minor: Is there a reason that these ones have cmp::partial_min(2, 2) but the non-partial ones don't?)

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 thought I was making them all the same, but it's possible that i just screwed tha tup

@steveklabnik steveklabnik force-pushed the doc_core_cmp branch 2 times, most recently from 956558f to 7c63cd1 Compare February 11, 2015 03:37
@@ -302,6 +422,15 @@ pub fn partial_min<T: PartialOrd>(v1: T, v2: T) -> Option<T> {
/// Compare and return the maximum of two values if there is one.
///
/// Returns the first argument if the comparison determines them to be equal.
///
/// # Examples
Copy link
Member

Choose a reason for hiding this comment

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

This could have a NaN example too.

Copy link
Member Author

Choose a reason for hiding this comment

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

truth. fixed!

/// When comparison is impossible:
///
/// ```
/// let result = std::f64::NAN.partial_min(&1.0);
Copy link
Member

Choose a reason for hiding this comment

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

This and partial_max should be function calls, not methods.

@steveklabnik
Copy link
Member Author

@huonw should be good to go now

@huonw
Copy link
Member

huonw commented Feb 16, 2015

@bors r+ 17f9 rollup

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 17, 2015
Fix up, add examples, make them all the same.
@huonw huonw merged commit 17f9d36 into rust-lang:master Feb 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants