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.

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

Derive Eq for std::cmp::Ordering, instead of using manual impl. #95017

Merged
merged 2 commits into from
Mar 19, 2022

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Mar 16, 2022

This allows consts of type Ordering to be used in patterns, and with feature(adt_const_params) allows using Ordering as a const generic parameter.

Currently, std::cmp::Ordering implements Eq using a manually written impl Eq for Ordering {}, instead of derive(Eq). This means that it does not implement StructuralEq.

This commit removes the manually written impl, and adds derive(Eq) to Ordering, so that it will implement StructuralEq.

This allows consts of type Ordering to be used in patterns, and (with feature(adt_const_params)) allows using Orderings as const generic parameters.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2022
Added test in library/core/tests/cmp.rs that ensures that `const`s of type `Ordering`s can be used in patterns.
@zachs18
Copy link
Contributor Author

zachs18 commented Mar 16, 2022

The manual impl appears to have been added in 2013 with this commit ( a0f8540 ). As far as I can tell, derive(Eq) (deriving(TotalEq)) was possible then, so I don't know if there is a reason it was not derived at the time.

@zachs18 zachs18 marked this pull request as ready for review March 16, 2022 19:54
@Dylan-DPC
Copy link
Member

r? @Dylan-DPC

@rust-highfive rust-highfive assigned Dylan-DPC and unassigned kennytm Mar 18, 2022
@Dylan-DPC
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 18, 2022

📌 Commit b13b495 has been approved by Dylan-DPC

@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 18, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 18, 2022
…ylan-DPC

Derive Eq for std::cmp::Ordering, instead of using manual impl.

This allows consts of type Ordering to be used in patterns, and with feature(adt_const_params) allows using `Ordering` as a const generic parameter.

Currently, `std::cmp::Ordering` implements `Eq` using a manually written `impl Eq for Ordering {}`, instead of `derive(Eq)`. This means that it does not implement `StructuralEq`.

This commit removes the manually written impl, and adds `derive(Eq)` to `Ordering`, so that it will implement `StructuralEq`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#94115 (Let `try_collect` take advantage of `try_fold` overrides)
 - rust-lang#94295 (Always evaluate all cfg predicate in all() and any())
 - rust-lang#94848 (Compare installed browser-ui-test version to the one used in CI)
 - rust-lang#94993 (Add test for >65535 hashes in lexing raw string)
 - rust-lang#95017 (Derive Eq for std::cmp::Ordering, instead of using manual impl.)
 - rust-lang#95058 (Add use of bool::then in sys/unix/process)
 - rust-lang#95083 (Document that `Option<extern "abi" fn>` discriminant elision applies for any ABI)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4ead6d9 into rust-lang:master Mar 19, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 19, 2022
@m-ou-se m-ou-se added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 27, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

This needed a @rust-lang/libs-api FCP, as this was a change to the public stable API.

This change has already shipped stably as part of 1.61. Hopefully everyone agrees this change was uncontroversial.

@BurntSushi
Copy link
Member

@m-ou-se What was the public API change here? Is my mental model that derive(Eq) is precisely equivalent to impl Eq for Ordering wrong? (At least, for this specific case.)

@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

This adds an implementation of StructuralEq. (See the top comment.)

@BurntSushi
Copy link
Member

Oh wow, okay, that's subtle. Hopefully we can figure out how to make such changes more explicit in the future.

But I think this specific change looks okay at least.

@dtolnay
Copy link
Member

dtolnay commented Jun 27, 2022

fn main() {
    const ORDERING: std::cmp::Ordering = std::cmp::Ordering::Less;
    if let ORDERING = ORDERING {}
}
$ cargo +1.61.0 check
    Checking repro v0.0.0
    Finished dev [unoptimized + debuginfo] target(s) in 0.66s

$ cargo +1.60.0 check
    Checking repro v0.0.0

error: to use a constant of type `std::cmp::Ordering` in a pattern, `std::cmp::Ordering` must be annotated with `#[derive(PartialEq, Eq)]`
 --> src/main.rs:3:12
  |
3 |     if let ORDERING = ORDERING {}
  |            ^^^^^^^^

warning: irrefutable `if let` pattern
 --> src/main.rs:3:8
  |
3 |     if let ORDERING = ORDERING {}
  |        ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(irrefutable_let_patterns)]` on by default
  = note: this pattern will always match, so the `if let` is useless
  = help: consider replacing the `if let` with a `let`

error: could not compile `repro` due to previous error; 1 warning emitted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants