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

RFC: Add a partial_cmp method to PartialOrd #100

Merged
merged 2 commits into from
Jun 17, 2014

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented Jun 1, 2014

No description provided.

@alexcrichton
Copy link
Member

I find it kinda nice that it would bring the PartialOrd trait in line with the Ord trait in terms of what it provides (under the proposed system). I may lean towards PartialLess instead of PartialOrdLess in terms of naming.

The default implementations could always be overridden if performance is necessary, so I wouldn't see the concern of the default implementation providing a possibly slow default being too worrisome.

@sfackler
Copy link
Member Author

sfackler commented Jun 2, 2014

I'm using PartialOrdLess because if the thing at the bottom happens as well, both PartialOrdering and PartialEquality would have a variant named PartialEqual otherwise. If we're not going to add partial_eq then we can drop the Ord from the variants.

@glaebhoerl
Copy link
Contributor

Why not just Option<Ordering>?

@huonw
Copy link
Member

huonw commented Jun 2, 2014

@glaebhoerl that is currently 2 bytes. (But implementing an optimisation like rust-lang/rust#14540 would fix that small issue.)

@sfackler
Copy link
Member Author

sfackler commented Jun 2, 2014

@glaebhoerl oh, that's a good idea. I'm not super concerned about a 1 byte difference in representation size. I can't imagine people storing millions of partial ordering results or anything.

I'd prefer to keep an enum around for partial_eq as Option<bool> doesn't seem to carry the same meaning as the PartialEquality enum in my eyes.

@glaebhoerl
Copy link
Contributor

I'd prefer to keep an enum around for partial_eq as Option<bool> doesn't seem to carry the same meaning as the PartialEquality enum in my eyes.

I have the same feeling. Something like enum IsItEqual { DefinitelyEqual, DefinitelyInequal } and then you can put that in an Option for great symmetry.

@sfackler
Copy link
Member Author

sfackler commented Jun 3, 2014

Updated in line with @glaebhoerl's comments.

@sfackler
Copy link
Member Author

sfackler commented Jun 4, 2014

Here's an example of use of partial_cmp to help with some nontrivial PartialOrd impls: sfackler/rust-postgres@21feed4

@alexcrichton
Copy link
Member

We decided to accept this RFC, discussed in today's meeting

@Centril Centril added the A-compare Proposals relating to comparisons. label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compare Proposals relating to comparisons.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants