Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Optimization for bool's PartialOrd impl #80034

Closed
ChayimFriedman2 opened this issue Dec 14, 2020 · 1 comment · Fixed by #80035
Closed

Optimization for bool's PartialOrd impl #80034

ChayimFriedman2 opened this issue Dec 14, 2020 · 1 comment · Fixed by #80035
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ChayimFriedman2
Copy link
Contributor

#66780 suggested an optimization for impl Ord for bool, and it was implemented in #66881.

However, for some reason, impl PartialOrd for bool was not optimized the same way:

rust/library/core/src/cmp.rs

Lines 1286 to 1300 in 1f7762b

impl Ord for bool {
#[inline]
fn cmp(&self, other: &bool) -> Ordering {
// Casting to i8's and converting the difference to an Ordering generates
// more optimal assembly.
// See <https://github.com/rust-lang/rust/issues/66780> for more info.
match (*self as i8) - (*other as i8) {
-1 => Less,
0 => Equal,
1 => Greater,
// SAFETY: bool as i8 returns 0 or 1, so the difference can't be anything else
_ => unsafe { unreachable_unchecked() },
}
}
}

rust/library/core/src/cmp.rs

Lines 1236 to 1241 in 1f7762b

impl PartialOrd for bool {
#[inline]
fn partial_cmp(&self, other: &bool) -> Option<Ordering> {
(*self as u8).partial_cmp(&(*other as u8))
}
}

It can be implemented in terms of Ord:

    impl PartialOrd for bool {
        #[inline]
        fn partial_cmp(&self, other: &bool) -> Option<Ordering> {
            Some(self.cmp(other))
        }
    }

Is this deliberate or just something that no-one noticed?

@camelid
Copy link
Member

camelid commented Dec 14, 2020

By the way, you don't need to open an issue if you're about to open a PR. Just include what you would put in the issue in the PR!

@camelid camelid added T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Dec 14, 2020
@camelid camelid changed the title impl PartialOrd for bool: optimization Optimization for bool's PartialOrd impl Dec 14, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 15, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 15, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 15, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 16, 2020
@bors bors closed this as completed in 93f1c67 Dec 17, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants