-
Notifications
You must be signed in to change notification settings - Fork 13.4k
float tests: deduplicate min, max, and rounding tests #142243
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
base: master
Are you sure you want to change the base?
Conversation
assert_biteq!((-0.0 as Float).midpoint(-0.0), -0.0); | ||
assert_biteq!((-5.0 as Float).midpoint(5.0), 0.0); | ||
assert_biteq!(Float::MAX.midpoint(Float::MIN), 0.0); | ||
assert_biteq!(Float::MIN.midpoint(Float::MAX), 0.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.
The old test seemed to assume that the result of MIN.midpoint(MAX)
should be -0.0
, but actually we return 0.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.
The midpoint
function in C++ with clang and GCC also seems to return +0.0, so this should be fine. We don't guarantee the sign of zero anyway for midpoint
.
dc526c1
to
7171984
Compare
7171984
to
1b6bb45
Compare
assert_eq!(Float::MAX.ceil(), Float::MAX); | ||
assert_eq!(Float::MIN.ceil(), Float::MIN); | ||
assert_eq!(Float::MIN_POSITIVE.ceil(), 1.0); | ||
assert_eq!((-Float::MIN_POSITIVE).ceil(), 0.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.
A bunch of the -Float::MIN_POSITIVE
rounding tests were missing the -
as well.
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.
LGTM, thanks for the cleanup. One question then r=me.
@@ -14,10 +37,11 @@ macro_rules! assert_approx_eq { | |||
); | |||
}}; | |||
} | |||
pub(crate) use assert_approx_eq_ as assert_approx_eq; |
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.
What is the reason for the trailing _
rather than just pub(crate) use assert_approx_eq;
? I assume there may be some name conflicts.
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.
I got an error about name clashes:
error[E0659]: `assert_approx_eq` is ambiguous
--> library/coretests/tests/floats/mod.rs:681:9
|
681 | assert_approx_eq!((-1.7 as Float).fract(), -0.7);
| ^^^^^^^^^^^^^^^^ ambiguous name
|
= note: ambiguous because of a conflict between a `macro_rules` name and a non-`macro_rules` name from another module
I think I'll pick actually proper names for both runtime and const macros and use imports for name management, that's probably clearer.
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.
I restructured the macros a bit to make it more clear (I think), please take a look. Putting the const ones in a module doesn't work out that well since (a) macros have their own special global namespace, and (b) we can't glob-import them anyway.
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.
Yeah that looks good to me. Maybe just add a breadcrumb comment at the use {assert_*_const as assert_*}
remapping? Also if you don't mind grabbing it while you're here, I notice the preexisting doc example for the float_test
macro is missing its closing /// ```
.
I still can't wrap my head around the way macro namespaces work sometimes...
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.
There's already a comment there. What would you like to have added to it?
I still can't wrap my head around the way macro namespaces work sometimes...
Yeah.^^ I wonder if we can get rid of it in a future edition...
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.
There is a comment at the assert_approx_eq_rt as assert_approx_eq
version mentioning that the rt version is used by default so const can shadow it, I was just thinking to mirror this at the place we shadow it (lines 193-194). But it's noncritical, r=me with or without it.
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.
Oops forgot the ✅
d8240ac
to
a2584a4
Compare
a2584a4
to
6da4f58
Compare
Part of #141726
Best reviewed commit-by-commit.
assert_biteq!
in themod.rs
tests. This requires some trickery to make shadowing macros with imports work.tests/floats/f*.rs
are entirely subsumed by what we already have intests/float/mod.rs
, so I just removed them.f*.rs
had more test points, so I copied them over. They didn't have0.5
and-0.5
though which seem like interesting points in particular regarding the sign of the resulting zero if that's what it sounds to, and they didn't max min/max/inf/nan tests, so this was really a merger of both tests.r? @tgross35