Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Override PartialOrd methods for bool #138945

Merged
merged 1 commit into from
Mar 26, 2025
Merged

Override PartialOrd methods for bool #138945

merged 1 commit into from
Mar 26, 2025

Conversation

DaniPopes
Copy link
Contributor

I noticed that PartialOrd implementation for bool does not override the individual operator methods, unlike the other primitive types like char and integers.

This commit extracts these PartialOrd overrides shared by the other primitive types into a macro and calls it on bool too.

CC @scottmcm for our recent adventures in PartialOrd land

@DaniPopes
Override PartialOrd methods for bool
154cb08 
I noticed that `PartialOrd` implementation for `bool` does not override the
individual operator methods, unlike the other primitive types like `char`
and integers.

This commit extracts these `PartialOrd` overrides shared by the other
primitive types into a macro and calls it on `bool` too.
@rustbot
Copy link
Collaborator

rustbot commented Mar 25, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 25, 2025
@scottmcm scottmcm assigned scottmcm and unassigned Mark-Simulacrum Mar 25, 2025
scottmcm
scottmcm reviewed Mar 25, 2025
View reviewed changes
library/core/src/cmp.rs
@@ -1912,6 +1914,8 @@ mod impls {
fn partial_cmp(&self, other: &bool) -> Option<Ordering> {
Some(self.cmp(other))
}

partial_ord_methods_primitive_impl!();
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I was also adding the chaining ones here in https://github.com/rust-lang/rust/pull/138881/files#diff-792af5fefd1cfdfab121523d464c37a6793eea3b804ac5a9cb3aaa8aa1b2ade4R1916, but I like this version instead that merges the lt/le/etc in too.

I'll rebase mine atop yours; this can go in first.

@scottmcm
Copy link
Member

Thanks! Definitely agreed that the current behaviour https://rust.godbolt.org/z/s14bshzGE where we get

core::cmp::PartialOrd::lt::ha3b3e39dc7c4e4d4:
        sub     rsp, 40
        mov     qword ptr [rsp + 16], rdi
        mov     qword ptr [rsp + 24], rsi
        call    core::cmp::impls::<impl core::cmp::PartialOrd for bool>::partial_cmp::h0dc623f7b1df1961
        mov     byte ptr [rsp + 15], al
        mov     eax, 1
        xor     ecx, ecx
        cmp     byte ptr [rsp + 15], 2
        cmove   rax, rcx
        test    rax, 1
        je      .LBB0_2
        mov     al, byte ptr [rsp + 15]
        mov     byte ptr [rsp + 39], al
        cmp     al, 0
        setl    al
        and     al, 1
        mov     byte ptr [rsp + 14], al
        jmp     .LBB0_3
.LBB0_2:
        mov     byte ptr [rsp + 14], 0
.LBB0_3:
        mov     al, byte ptr [rsp + 14]
        and     al, 1
        add     rsp, 40
        ret

is silly.

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Mar 25, 2025

📌 Commit 154cb08 has been approved by scottmcm

It is now in the queue for this repository.

@bors 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 Mar 25, 2025
@jhpratt jhpratt mentioned this pull request Mar 26, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2025
@bors
Auto merge of rust-lang#138956 - jhpratt:rollup-6g7ppwd, r=jhpratt
6e8abb5 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#138128 (Stabilize `#![feature(precise_capturing_in_traits)]`)
 - rust-lang#138834 (Group test diffs by stage in post-merge analysis)
 - rust-lang#138867 (linker: Fix staticlib naming for UEFI)
 - rust-lang#138874 (Batch mark waiters as unblocked when resuming in the deadlock handler)
 - rust-lang#138875 (Trusty: Fix build for anonymous pipes and std::sys::process)
 - rust-lang#138877 (Ignore doctests only in specified targets)
 - rust-lang#138885 (Fix ui pattern_types test for big-endian platforms)
 - rust-lang#138905 (Add target maintainer information for powerpc64-unknown-linux-musl)
 - rust-lang#138911 (Allow defining opaques in statics and consts)
 - rust-lang#138917 (rustdoc: remove useless `Symbol::is_empty` checks.)
 - rust-lang#138945 (Override PartialOrd methods for bool)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit deb987b into rust-lang:master Mar 26, 2025
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2025
@rust-timer
Unrolled build for rust-lang#138945
3c19993 
Rollup merge of rust-lang#138945 - DaniPopes:override-partialord-bool, r=scottmcm

Override PartialOrd methods for bool

I noticed that `PartialOrd` implementation for `bool` does not override the individual operator methods, unlike the other primitive types like `char` and integers.

This commit extracts these `PartialOrd` overrides shared by the other primitive types into a macro and calls it on `bool` too.

CC `@scottmcm` for our recent adventures in `PartialOrd` land
@rustbot rustbot added this to the 1.87.0 milestone Mar 26, 2025
@DaniPopes DaniPopes deleted the override-partialord-bool branch March 26, 2025 06:55
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 2, 2025
@jhpratt
Rollup merge of rust-lang#138945 - DaniPopes:override-partialord-bool…
51a22a0 
…, r=scottmcm

Override PartialOrd methods for bool

I noticed that `PartialOrd` implementation for `bool` does not override the individual operator methods, unlike the other primitive types like `char` and integers.

This commit extracts these `PartialOrd` overrides shared by the other primitive types into a macro and calls it on `bool` too.

CC `@scottmcm` for our recent adventures in `PartialOrd` land
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 2, 2025
@bors
Auto merge of rust-lang#138956 - jhpratt:rollup-6g7ppwd, r=jhpratt
b8b3b41 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#138128 (Stabilize `#![feature(precise_capturing_in_traits)]`)
 - rust-lang#138834 (Group test diffs by stage in post-merge analysis)
 - rust-lang#138867 (linker: Fix staticlib naming for UEFI)
 - rust-lang#138874 (Batch mark waiters as unblocked when resuming in the deadlock handler)
 - rust-lang#138875 (Trusty: Fix build for anonymous pipes and std::sys::process)
 - rust-lang#138877 (Ignore doctests only in specified targets)
 - rust-lang#138885 (Fix ui pattern_types test for big-endian platforms)
 - rust-lang#138905 (Add target maintainer information for powerpc64-unknown-linux-musl)
 - rust-lang#138911 (Allow defining opaques in statics and consts)
 - rust-lang#138917 (rustdoc: remove useless `Symbol::is_empty` checks.)
 - rust-lang#138945 (Override PartialOrd methods for bool)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmcm scottmcm scottmcm left review comments

Assignees

@scottmcm scottmcm

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
5 participants
@DaniPopes @rustbot @scottmcm @bors @Mark-Simulacrum