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

require a total ordering for Eq and Ord #12435

Closed
wants to merge 1 commit into from
Closed

require a total ordering for Eq and Ord #12435

wants to merge 1 commit into from

Conversation

thestinger
Copy link
Contributor

A total ordering is assumed and required for most generic functions
making use of these traits. The cmp method also provides a boost in
efficiency for sorting sequence types or storing them in a tree set/map
since otherwise two passes would often be required.

The standard hardware comparison operators for floats do not implement a
total order. However, IEEE754 does define an alternate total ordering.
Fixing this is now issue #12434 and doesn't need to stand in the way of
a sane comparison module.

Traits like PartialEq and PartialOrd could exist, but will never
permit a sane implementation. A type implementing Eq would be expected
to automatically implement PartialEq but floating point types have
two comparison definitions available. I don't think these would see
much usage anyway, because there are even alternate definitions of
functions as simple as min, max and clamp for floats.

Closes #5283
Closes #8071
Closes #10320

@omasanori
Copy link
Contributor

How about types not providing strict total ordering in Ord?

@thestinger
Copy link
Contributor Author

I opened an issue about floating point types: #12434, and I don't think there are other problems.

@omasanori
Copy link
Contributor

AFAIK lt for semver::Version provides strict weak ordering in line with the spec.

For reference:
Wikipedia's article on weak ordering
Strict Weak Ordering - SGI (C++ requires strict weak ordering for sorting, comparison of keys of maps, etc.)

@brson
Copy link
Contributor

brson commented Feb 21, 2014

@thestinger thanks for pushing on this.

@brson
Copy link
Contributor

brson commented Feb 21, 2014

Does this also make the built in comparison operators for floats a total ordering?

@thestinger
Copy link
Contributor Author

Rust requires a total ordering for the sort function and TreeMap since it makes use of a cmp method returning one of Less, Greater or Equal. I don't think there's much of a use case for a strict weak ordering trait in Rust's standard library.

@brson: No, I haven't touched the floating point implementations yet. At the moment it's just an incorrect implementation, but since std::hashmap never actually used TotalEq it's not much of a step backwards. I opened a separate bug about floating point.

@omasanori: If Semantic Versioning is unable to provide a basic guarantee like !(a < b) && !(b < a) implying that a and b are equal, then I don't think we should be using Semantic Versioning as that seems insane. If the build id is not considered to be relevant to version equality, then it should be considered separate from the version. Floating point at least has the excuse of NaN representing a lack of information, like SQL NULL.

* Requirements:
*
* `a != b` returns the same value as `!(a == b)`
* `a == a` is true
Copy link
Member

Choose a reason for hiding this comment

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

Can we put "(reflexive)" "(symmetric)" and "(transitive)" for the three properties below here (since that's the jargon for each of them).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll add in the properties in addition to the examples. This isn't a very exhaustive list of requirements anyway, but at least it gets the point across!

@thestinger
Copy link
Contributor Author

Floating point types don't actually satisfy the C++ weak total ordering definition required by std::map... it's undefined to insert a NaN key into a tree set/map or a hash set/map. I don't really see a point of traits if we're not encoding the requirements into the traits...

@thestinger
Copy link
Contributor Author

@omasanori: Could we just leave out build metadata as a whole? It seems optional, and that'd be an easy way out of the problem.

@omasanori
Copy link
Contributor

I'm sorry, I removed a comment accidentally. What I said was the problem is in build metadata.

@thestinger Probably we can. Semver doesn't define equality, so it will be OK.

@pcwalton
Copy link
Contributor

Cool, this is very similar to what I had in mind. One thing I was wondering about is why the default method is lt and not cmp. Seems to me that cmp is more efficient for large data structures because it gives you more information without having to do two comparisons.

@thestinger
Copy link
Contributor Author

@pcwalton: It would be nice to define lt, gt, le, ge and cmp in a cycle to allow implementing only one of the methods and then add something like a lint to warn if at least one is not implemented. At the moment I'm just doing an override of cmp and mentioning it in the documentation as something you should override when you can perform a single pass, or when it's a generic wrapper. Deriving now does the right thing and calls the underlying cmp too.

@huonw
Copy link
Member

huonw commented Feb 22, 2014

a lint to warn if at least one is not implemented

#7771.

A total ordering is assumed and required for most generic functions
making use of these traits. The `cmp` method also provides a boost in
efficiency for sorting sequence types or storing them in a tree set/map
since otherwise two passes would often be required.

The standard hardware comparison operators for floats do not implement a
total order. However, IEEE754 does define an alternate total ordering.
Fixing this is now issue #12434 and doesn't need to stand in the way of
a sane comparison module.

Traits like `PartialEq` and `PartialOrd` could exist, but will never
permit a sane implementation. A type implementing `Eq` would be expected
to automatically implement `PartialEq` but floating point types have
*two* comparison definitions available. I don't think these would see
much usage anyway, because there are even alternate definitions of
functions as simple as `min`, `max` and `clamp` for floats.

Closes #5283
Closes #8071
Closes #10320
@thestinger thestinger closed this Feb 24, 2014
@thestinger
Copy link
Contributor Author

I am not interested in playing politics anymore so I have no intention to try to land this.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
…eykril

[editors/vscode] cleaer status bar bg color / command  when server status returns to OK

fixes rust-lang#12433
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 4, 2024
don't lint [`mixed_attributes_style`] when mixing docs and other attrs

add test files for issue rust-lang#12436

move [`mixed_attributes_style`] to `LateLintPass` to enable global `allow`

stop [`mixed_attributes_style`] from linting on different attributes

add `@compile-flags` to [`mixed_attributes_style`]'s test;

turns out not linting in test mod is not a FN.

Apply suggestions from code review

Co-authored-by: Timo <[email protected]>

move [`mixed_attributes_style`] to late pass and stop it from linting on different kind of attributes
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 4, 2024
don't lint [`mixed_attributes_style`] when mixing docs and other attrs

fixes: rust-lang#12435
fixes: rust-lang#12436
fixes: rust-lang#12530

---

changelog: don't lint [`mixed_attributes_style`] when mixing different kind of attrs; and move it to late pass;
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 25, 2024
don't lint [`mixed_attributes_style`] when mixing docs and other attrs

fixes: rust-lang#12435
fixes: rust-lang#12436
fixes: rust-lang#12530

---

changelog: don't lint [`mixed_attributes_style`] when mixing different kind of attrs; and move it to late pass;
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 25, 2024
don't lint [`mixed_attributes_style`] when mixing docs and other attrs

fixes: rust-lang#12435
fixes: rust-lang#12436
fixes: rust-lang#12530

---

changelog: don't lint [`mixed_attributes_style`] when mixing different kind of attrs; and move it to late pass;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants