-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Re-implement float min/max in rust #42430
Conversation
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
// Since we do not support sNaN in Rust yet, we do not need to handle them. | ||
// FIXME(nagisa): due to https://bugs.llvm.org/show_bug.cgi?id=33303 we canonicalize by | ||
// multiplying by 1.0. Should switch to the `canonicalize` when it works. | ||
(if self < other || other.is_nan() { self } else { other }) * 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean self > other
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self < other
so self
is that minimal one, seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other one is/was wrong. I’ll just write down some tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7544f5c
to
540373d
Compare
Tidy is failing because this isn't mentioned in the book. |
333bf76
to
ef2b445
Compare
Aturon's on vacation this week, so let's try... |
src/libcore/num/mod.rs
Outdated
#[unstable(feature = "core_float_min_max", issue="0")] | ||
fn max(self, other: Self) -> Self; | ||
/// Returns the minimum of the two numbers. | ||
#[unstable(feature = "core_float_min_max", issue="0")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are stable in the standard library I think it's fine to go ahead and stabilize them in libcore itself, this is just moving the implementation down a layer where we could
// Since we do not support sNaN in Rust yet, we do not need to handle them. | ||
// FIXME(nagisa): due to https://bugs.llvm.org/show_bug.cgi?id=33303 we canonicalize by | ||
// multiplying by 1.0. Should switch to the `canonicalize` when it works. | ||
(if self < other || self.is_nan() { other } else { self }) * 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the musl implementation is quite different, it's not clear to me that these are the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference between the implementations is handling of the signed zero. IEEE 754 allows to ignore the signed-ness of zero here and, if 0.0
is compared to -0.0
, to return either one.
Even the C99 standard does not mention signed zeroes in the annex referenced by musl. It quite literally paraphrases the IEEE 754 and provides an example implementation which matches (with small differences) the one provided here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok seems reasonable!
bd0be22
to
b1dadc8
Compare
@bors: r+ |
📌 Commit ba6cf1d has been approved by |
Re-implement float min/max in rust This also adds the relevant implementations into libcore. See #42423
☀️ Test successful - status-appveyor, status-travis |
This also adds the relevant implementations into libcore.
See #42423