Move several float tests to floats/mod.rs #143738

rocurley · 2025-07-10T16:36:11Z

This PR moves several tests to floats/mod.rs, as discussed in #141726. The tests moved are:

test_num_f*
test_infinity
test_neg_infinity
test_zero
test_neg_zero
test_one
test_is_nan
test_is_infinite
test_is_finite
test_is_normal
test_classify

Each test is its own commit, so it may be easiest to review each commit individually.

r? tgross35

rocurley · 2025-07-10T16:37:46Z

library/coretests/tests/floats/mod.rs

@@ -20,6 +19,31 @@ impl Approx for f128 {
    const LIM: Self = 1e-9;
 }

+trait TestableFloat {


Possibly we should merge Approx into this?

Alternatively, there could be separate floats for each logical grouping of constants. I don't really like that because it seems like a lot of boilerplate, but it would mean the traits here more meaningful.

Moving LIM to TestableFloat sounds good to me 👍 Maybe rename it APPROX there.

tgross35 · 2025-07-10T19:06:18Z

library/coretests/tests/floats/mod.rs

@@ -20,6 +19,31 @@ impl Approx for f128 {
    const LIM: Self = 1e-9;
 }

+trait TestableFloat {


Moving LIM to TestableFloat sounds good to me 👍 Maybe rename it APPROX there.

tgross35 · 2025-07-10T19:08:06Z

library/coretests/tests/floats/mod.rs

+    test<Float> {
+    let neg_zero: Float = -0.0;
+    assert!(0.0 == neg_zero);
+    assert_biteq!(-0.0, neg_zero);
+    assert!(!neg_zero.is_infinite());
+    assert!(neg_zero.is_finite());
+    assert!(!neg_zero.is_sign_positive());
+    assert!(neg_zero.is_sign_negative());
+    assert!(!neg_zero.is_nan());
+    assert!(!neg_zero.is_normal());
+    assert!(matches!(neg_zero.classify(), Fp::Zero));
+    }


Could you indent this to match?

tgross35 · 2025-07-11T00:11:38Z

library/coretests/tests/floats/mod.rs

+impl TestableFloat for f16 {
+    const SMALL_NORMAL: Self = 1e-4;
+    const SUBNORMAL: Self = 1e-5;
+}
+
+impl TestableFloat for f32 {
+    const SMALL_NORMAL: Self = 1e-37;
+    const SUBNORMAL: Self = 1e-38;
+}
+
+impl TestableFloat for f64 {
+    const SMALL_NORMAL: Self = 1e-307;
+    const SUBNORMAL: Self = 1e-308;
+}
+
+impl TestableFloat for f128 {
+    const SMALL_NORMAL: Self = 1e-4931;
+    const SUBNORMAL: Self = 1e-4932;
+}


Could you rename SMALL_NORMAL to MIN_POSITIVE_NORMAL? Then these can just be f32::MIN_POSITIVE rather than a handwritten value. You could also use MIN_POSITIVE directly, but that name is confusing.

SUBNORMAL can similarly be renamed to MAX_SUBNORMAL with a value f32::MIN_POSITIVE.next_down().

rocurley · 2025-07-11T02:11:14Z

@rustbot review

tgross35

Couple of other small things.

After this, please clean up history by squashing the fixup commits. Or just squash everything together, if that's easier.

tgross35 · 2025-07-11T02:52:45Z

library/coretests/tests/floats/mod.rs

+        assert!(!0.0f32.is_nan());
+        assert!(!5.3f32.is_nan());
+        assert!(!(-10.732f32).is_nan());


These are all f32 so you'll need to bind them to a variable of type Float

tgross35 · 2025-07-11T02:55:01Z

library/coretests/tests/floats/mod.rs

+        assert!(matches!(neg_inf.classify(), Fp::Infinite));
+        assert!(matches!(zero.classify(), Fp::Zero));
+        assert!(matches!(neg_zero.classify(), Fp::Zero));
+        assert!(matches!(1f32.classify(), Fp::Normal));


Same here, this isn't the right type.

If it makes things easier you could add ZERO, ONE, NEG_ZERO and NEG_ONE to the trait since those specific values are used in multiple functions.

rocurley · 2025-07-11T14:47:19Z

Oof, thanks for catching those. Should be fixed now, and I've squashed the fixes into the appropriate original commits.

It seems like keeping the commit history clean is important here. I'm curious about the motivation: are the PR commits not squashed when merging into master?

@rustbot review

tgross35 · 2025-07-14T05:55:00Z

It seems like keeping the commit history clean is important here. I'm curious about the motivation: are the PR commits not squashed when merging into master?

They are not, the option would be nice but unfortunately Bors doesn't support it. Feel free to squash this if you get the chance before Bors picks it up since the commits are pretty tiny. No worries if you don't get to it since at least the history is clean.

@bors r+

bors · 2025-07-14T05:55:03Z

📌 Commit 79769f2 has been approved by tgross35

It is now in the queue for this repository.

…=tgross35 Move several float tests to floats/mod.rs This PR moves several tests to `floats/mod.rs`, as discussed in rust-lang#141726. The tests moved are: - `test_num_f*` - `test_infinity` - `test_neg_infinity` - `test_zero` - `test_neg_zero` - `test_one` - `test_is_nan` - `test_is_infinite` - `test_is_finite` - `test_is_normal` - `test_classify` Each test is its own commit, so it may be easiest to review each commit individually. r? tgross35

Rollup of 17 pull requests Successful merges: - #142885 (core: Add `BorrowedCursor::with_unfilled_buf`) - #143217 (Port #[link_ordinal] to the new attribute parsing infrastructure) - #143355 (wrapping shift: remove first bitmask and table) - #143448 (remote-test-client: Exit code `128 + <signal-number>` instead of `3`) - #143681 (bootstrap/miri: avoid rebuilds for test builds) - #143710 (Updates to random number generation APIs) - #143724 (Tidy cleanup) - #143738 (Move several float tests to floats/mod.rs) - #143820 (Fixed a core crate compilation failure when enabling the `optimize_for_size` feature on some targets) - #143850 (Compiletest: Simplify {Html,Json}DocCk directive handling) - #143855 (Port `#[omit_gdb_pretty_printer_section]` to the new attribute parsing) - #143868 (warn on align on fields to avoid breaking changes) - #143875 (update issue number for `const_trait_impl`) - #143881 (Use zero for initialized Once state) - #143887 (Run bootstrap tests sooner in the `x test` pipeline) - #143893 (Don't require `eh_personality` lang item on targets that have a personality) - #143901 (Region constraint nits) Failed merges: - #143878 (Port `#[pointee]` to the new attribute parsing infrastructure) - #143891 (Port `#[coverage]` to the new attribute system) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned tgross35 Jul 10, 2025

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 10, 2025

rocurley commented Jul 10, 2025

View reviewed changes

rocurley marked this pull request as ready for review July 10, 2025 16:38

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2025

tgross35 requested changes Jul 11, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 11, 2025

tgross35 reviewed Jul 11, 2025

View reviewed changes

rocurley added 11 commits July 11, 2025 10:31

Consolidate infinity tests

cfb66e5

Consolidate test_num tests

c5e67b4

Consolidate neg_infinity tests

c2e6b39

Consolidate zero tests

0c01322

Consolidate negative zero tests

fc01eed

Consolidate one tests

868020e

Consolidate is_nan

1b8904c

Consolidate is_infinite tests

e3d8367

Consolidate is_finite tests

7dd2811

Consolidate is_normal tests

d2c1900

Consolidate classify tests

79769f2

rocurley force-pushed the float_tests_refactor_2 branch from 261da66 to 79769f2 Compare July 11, 2025 14:41

tgross35 approved these changes Jul 14, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 14, 2025

jhpratt mentioned this pull request Jul 14, 2025

Rollup of 17 pull requests #143916

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Move several float tests to floats/mod.rs #143738

Move several float tests to floats/mod.rs #143738

rocurley commented Jul 10, 2025

Uh oh!

rocurley Jul 10, 2025 •

edited

Loading

Uh oh!

tgross35 Jul 10, 2025

Uh oh!

tgross35 Jul 10, 2025

Uh oh!

tgross35 Jul 10, 2025

Uh oh!

tgross35 Jul 11, 2025

Uh oh!

rocurley commented Jul 11, 2025

Uh oh!

tgross35 left a comment

Uh oh!

tgross35 Jul 11, 2025

Uh oh!

tgross35 Jul 11, 2025

Uh oh!

rocurley commented Jul 11, 2025

Uh oh!

tgross35 commented Jul 14, 2025

Uh oh!

bors commented Jul 14, 2025

Uh oh!

Uh oh!

Move several float tests to floats/mod.rs #143738

Are you sure you want to change the base?

Move several float tests to floats/mod.rs #143738

Conversation

rocurley commented Jul 10, 2025

Uh oh!

rocurley Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross35 Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

rocurley commented Jul 11, 2025

Uh oh!

tgross35 left a comment

Choose a reason for hiding this comment

Uh oh!

tgross35 Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

rocurley commented Jul 11, 2025

Uh oh!

tgross35 commented Jul 14, 2025

Uh oh!

bors commented Jul 14, 2025

Uh oh!

Uh oh!

rocurley Jul 10, 2025 •

edited

Loading