Skip to content

Mark lt from PartialOrd as inline #18090

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

Closed
ghost opened this issue Oct 16, 2014 · 7 comments
Closed

Mark lt from PartialOrd as inline #18090

ghost opened this issue Oct 16, 2014 · 7 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@ghost
Copy link

ghost commented Oct 16, 2014

pub trait PartialOrd: PartialEq {
    /// This method returns an ordering between `self` and `other` values
    /// if one exists.
    fn partial_cmp(&self, other: &Self) -> Option<Ordering>;

    /// This method tests less than (for `self` and `other`) and is used by the `<` operator.
    fn lt(&self, other: &Self) -> bool { ... }

    /// This method tests less than or equal to (`<=`).
    #[inline]
    fn le(&self, other: &Self) -> bool { ... }

    /// This method tests greater than (`>`).
    #[inline]
    fn gt(&self, other: &Self) -> bool { ... }

    /// This method tests greater than or equal to (`>=`).
    #[inline]
    fn ge(&self, other: &Self) -> bool { ... }
}

Shouldn't the lt method also be inline?

@nodakai
Copy link
Contributor

nodakai commented Oct 16, 2014

Before partial_cmp() was introduced, PartialOrd required only lt() and defined le(), gt() and ge() in terms of it. I agree now it should be #[inline], too.

@Gankra
Copy link
Contributor

Gankra commented Oct 16, 2014

(or that none of them should be inline, the eternal struggle)

@kmcallister kmcallister added A-libs E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Oct 16, 2014
@rclanan
Copy link
Contributor

rclanan commented Oct 18, 2014

On this same note, should eq also be marked as inline? On line 318 in cmp.rs it is but elsewhere it isn't.

fn eq(&self, _other: &()) -> bool { true }

@gamazeps
Copy link
Contributor

it already is ;)

@rclanan
Copy link
Contributor

rclanan commented Oct 18, 2014

Not on this line:

fn eq(&self, other: &Self) -> bool;

@Gankra
Copy link
Contributor

Gankra commented Oct 18, 2014

@rclanan The line you link is just a function delcaration, and not a function itself. I don't think you can (meaningfully) declare such a thing as #[inline].

@rclanan
Copy link
Contributor

rclanan commented Oct 19, 2014

Awesome, thanks for the feedback! I was just going based on what I saw on line 65 but I see the difference now.

bors added a commit that referenced this issue Oct 19, 2014
@ghost ghost closed this as completed Oct 21, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

5 participants