use apfloat for ldexp #902
Conversation
src/shims/foreign_items.rs
Outdated
| let n = unsafe { ldexp(x, exp) }; | ||
| this.write_scalar(Scalar::from_u64(n.to_bits()), dest)?; | ||
| // For radix-2 (binary) systems, `ldexp` and `scalbn` are the same. | ||
| let res = x.scalbn(exp.try_into().unwrap()); |
There was a problem hiding this comment.
@eddyb the odd thing is that the C version supports i32 but your version only i16. Panicking on big values seems wrong, any advise what else to do?
There was a problem hiding this comment.
Based on this snippet and the fact that f64 uses 11 bits of exponent and f128 would use 15 bits, you can safely clamp it between ExpInt::min_value() and ExpInt::max_value() and it would still be outside of the valid exponent range (so the effect will be the same, presumably underflow to 0 / overflow to Inf).
There was a problem hiding this comment.
Based on this snippet
So the only use of exp is in exp as i32? Seems more reasonable then to just expose it as an i32, IMO.
There was a problem hiding this comment.
But for now I went with clamping, thanks.
There was a problem hiding this comment.
i32 is only used as an intermediary to avoid overflow (specifically for f128, it could stay entirely within i16 for f64).
Most storage and manipulation of the exponent is done in ExpInt, which is i16.
Changing ExpInt to i32 would be possible (although wasteful IMO), it would just take more effort. And I'd prefer not to change only this one function (which I only ported for completeness sake, I didn't expect it to be useful).
|
@bors r+ |
|
📌 Commit 04892d9 has been approved by |
|
☀️ Test successful - checks-travis, status-appveyor |
No description provided.