[Variant] Allow lossless casting from integer to floating point #8357
Conversation
| @@ -144,3 +144,20 @@ pub(crate) const fn expect_size_of<T>(expected: usize) { | |||
| let _ = [""; 0][size]; | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) fn fits_precision<const N: u32>(n: impl Into<i64>) -> bool { | |||
There was a problem hiding this comment.
Put N into the generic, not the parameter, because this can yield better performance, do I understand correctly?
There was a problem hiding this comment.
It was mostly out of habit for small integer manipulation utilities... But now that you mention, it probably doesn't matter in the slightest -- the compiler will anyway inline aggressively and the constant arg will be folded in regardless of whether it's a generic arg or a function arg.
That said, there's one potential advantage to keeping the generic arg: Otherwise, it could be ambiguous which of two integer args is the precision and which is the actual value. Any preferences?
There was a problem hiding this comment.
Thanks for the detailed reply, I'm fine with the current implementation
| @@ -1096,13 +1096,21 @@ impl<'m, 'v> Variant<'m, 'v> { | |||
| /// let v2 = Variant::from(std::f64::consts::PI); | |||
| /// assert_eq!(v2.as_f16(), Some(f16::from_f64(std::f64::consts::PI))); | |||
| /// | |||
| /// // and from integers with no more than 11 bits of precision | |||
There was a problem hiding this comment.
Not sure if we need to add an overflow example here(e.g, Variant::from(2048) for f16)
There was a problem hiding this comment.
The unit test for fits_precision does test overflow, both positive and negative.
Do we need additional (indirect) test coverage here?
There was a problem hiding this comment.
Maybe we need to update the doc(the comment said that -- Returns Some(f16) for float and double variants, None for non-floating-point variants) or add an example for integer overflow (`Variant::from(2048) for f16 -- will return None)
I don't have a strong preference here, it's just an idea that popped into my mind when I saw this
There was a problem hiding this comment.
Oh! Good catch on the doc comment. Updated all three.
|
🚀 |
Which issue does this PR close?
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
Rationale for this change
Historically,
Variant::as_fXXmethods don't even try to cast int values as floating point, which is counter-intuitive.What changes are included in this PR?
Allow lossless casting of variant integer values to variant floating point values, by a naive determination of precision:
Are these changes tested?
New unit tests and doc tests.
Are there any user-facing changes?
Yes. Values that failed to cast before now succeed.