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

Implement total ord for Key #586

Merged
merged 2 commits into from
Sep 6, 2024
Merged

Implement total ord for Key #586

merged 2 commits into from
Sep 6, 2024

Conversation

mitsuhiko
Copy link
Owner

Fixes potential crashes on Rust 1.81.

@mitsuhiko
Copy link
Owner Author

Unfortuately rust-lang/rust#106418 does not exist yet, so std::mem::discriminant cannot be used.

@mitsuhiko mitsuhiko merged commit 8893db7 into master Sep 6, 2024
25 checks passed
@mitsuhiko mitsuhiko deleted the feature/total-ord branch September 6, 2024 09:09
This was referenced Sep 6, 2024
@orlp
Copy link

orlp commented Sep 6, 2024

@mitsuhiko I don't believe an explicit discriminant value was necessary here, I believe this would've sufficed:

if let (F64(l), F64(r)) = (self, other) {
    f64_total_cmp(*l, *r)
} else {
    self.partial_cmp(other).unwrap()
}

EDIT: never mind that's not true due to our overly aggressive requirements on Ord. We currently require
partial_cmp(a, b) == Some(cmp(a, b)) instead of the more relaxed partial_cmp(a, b) being Some implying equality to cmp(a, b).

You could still apply the above pattern but it's probably not worth it, you'd need a wrapper dummy struct just to derive PartialOrd on...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants