Skip to content

Conversation

@augustocesarperin
Copy link

@augustocesarperin augustocesarperin commented Jan 2, 2026

cc #141726

Unify the From tests from f16.rs and f128.rs into a single float_test! in mod.rs.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 2, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2026

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented Jan 3, 2026

@rustbot reroll

@rustbot rustbot assigned Mark-Simulacrum and unassigned jieyouxu Jan 3, 2026
@hkBst
Copy link
Member

hkBst commented Jan 3, 2026

cc @RalfJung @tgross35

Copy link
Member

@hkBst hkBst left a comment

Choose a reason for hiding this comment

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

assert_biteq!(f16::from(u8::MAX), 255.0);
assert_biteq!(f16::from(i8::MIN), -128.0);
assert_biteq!(f16::from(42_i8), 42.0);
assert_biteq!(f16::from(i8::MAX), 127.0);
Copy link
Member

Choose a reason for hiding this comment

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

Why only the bools, and not the rest of the cases here?

Copy link
Member

Choose a reason for hiding this comment

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

Probably because they want to know whether they are on the right track, given this is their first PR here.

Are you saying they are on the right track and should add more?

Copy link
Author

Choose a reason for hiding this comment

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

Tried to keep it minimal at first.
Added the u8/i8 cases now

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 4, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 4, 2026
@tgross35
Copy link
Contributor

tgross35 commented Jan 5, 2026

I believe this will be superseded by #148206

@augustocesarperin
Copy link
Author

@rustbot ready

@rustbot 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 Jan 6, 2026
@augustocesarperin
Copy link
Author

@tgross35 I believe both PRs contribute to #141726 but address different categories of tests. Please let me know if I'm missing something or if there's a better way to coordinate these changes.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

@tgross35 I believe both PRs contribute to #141726 but address different categories of tests. Please let me know if I'm missing something or if there's a better way to coordinate these changes.

Ah sorry, you're right!

View changes since this review

Comment on lines 1586 to 1637
float_test! {
name: from_bool,
attrs: {
f16: #[cfg(any(miri, target_has_reliable_f16))],
f128: #[cfg(any(miri, target_has_reliable_f128))],
},
test<Float> {
assert_biteq!(Float::from(false), Float::ZERO);
assert_biteq!(Float::from(true), Float::ONE);
}
}

float_test! {
name: from_u8,
attrs: {
f16: #[cfg(any(miri, target_has_reliable_f16))],
f128: #[cfg(any(miri, target_has_reliable_f128))],
},
test<Float> {
assert_biteq!(Float::from(u8::MIN), Float::ZERO);
assert_biteq!(Float::from(42_u8), 42.0);
assert_biteq!(Float::from(u8::MAX), 255.0);
}
}

float_test! {
name: from_i8,
attrs: {
f16: #[cfg(any(miri, target_has_reliable_f16))],
f128: #[cfg(any(miri, target_has_reliable_f128))],
},
test<Float> {
assert_biteq!(Float::from(i8::MIN), -128.0);
assert_biteq!(Float::from(42_i8), 42.0);
assert_biteq!(Float::from(i8::MAX), 127.0);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to split the tests up, leaving them in a single function as they are currently is fine.

Copy link
Author

Choose a reason for hiding this comment

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

I kept bool/u8/i8 together, but had to split u16/u32 since f16/f32 lack those From impls, otherwise it fails with E0277.

Comment on lines 23 to 41
assert_biteq!(f128::from(u16::MIN), 0.0);
assert_biteq!(f128::from(42_u16), 42.0);
assert_biteq!(f128::from(u16::MAX), 65535.0);
assert_biteq!(f128::from(i16::MIN), -32768.0);
assert_biteq!(f128::from(42_i16), 42.0);
assert_biteq!(f128::from(i16::MAX), 32767.0);
assert_biteq!(f128::from(u32::MIN), 0.0);
assert_biteq!(f128::from(42_u32), 42.0);
assert_biteq!(f128::from(u32::MAX), 4294967295.0);
assert_biteq!(f128::from(i32::MIN), -2147483648.0);
assert_biteq!(f128::from(42_i32), 42.0);
assert_biteq!(f128::from(i32::MAX), 2147483647.0);
// FIXME(f16_f128): Uncomment these tests once the From<{u64,i64}> impls are added.
// assert_eq!(f128::from(u64::MIN), 0.0);
// assert_eq!(f128::from(42_u64), 42.0);
// assert_eq!(f128::from(u64::MAX), 18446744073709551615.0);
// assert_eq!(f128::from(i64::MIN), -9223372036854775808.0);
// assert_eq!(f128::from(42_i64), 42.0);
// assert_eq!(f128::from(i64::MAX), 9223372036854775807.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Bring these tests over as well. Just gate them as needed:

if Float::BITS >= 32 {
    // ...
}

if Float::BITS >= 64 {
    // ...
}

if Float::BITS >= 128 {
    // ...
}

Copy link
Author

Choose a reason for hiding this comment

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

I see RawFloat has BITS but TestableFloat doesn't, and the f16 lacks From<u16/u32> so trait bounds still fail. Is cfg(false) ok here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This entire file can be deleted since it is no longer used. Ditto for f128 with the above change.

Copy link
Author

@augustocesarperin augustocesarperin Jan 13, 2026

Choose a reason for hiding this comment

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

Done! Also I'll update the PR title since it's not only From<bool>

@rustbot 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 Jan 11, 2026
@tgross35
Copy link
Contributor

r? tgross35

@rustbot rustbot assigned tgross35 and unassigned Mark-Simulacrum Jan 11, 2026
@augustocesarperin augustocesarperin changed the title Unify and deduplicate From<bool> float tests Unify and deduplicate From float tests Jan 13, 2026
@augustocesarperin augustocesarperin changed the title Unify and deduplicate From float tests Unify and deduplicate From<T> float tests Jan 13, 2026
@augustocesarperin
Copy link
Author

@rustbot ready

@rustbot 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 Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants