Support single precision floats in grisu formatting #1361
Conversation
vitaut
left a comment
There was a problem hiding this comment.
Thanks for another great PR! A few comments inline.
include/fmt/format-inl.h
Outdated
| } | ||
|
|
||
| void compute_float_boundaries(fp& lower, fp& upper) const { | ||
| constexpr significand_type half_step = |
There was a problem hiding this comment.
Does step mean ulp here? If yes, I recommend using ulp instead of step since it's an established term.
include/fmt/format-inl.h
Outdated
| static FMT_CONSTEXPR_DECL const int double_significand_size = | ||
| std::numeric_limits<double>::digits - 1; | ||
| static FMT_CONSTEXPR_DECL const int float_significand_size = | ||
| std::numeric_limits<float>::digits - 1; |
There was a problem hiding this comment.
I suggest replacing these two with a single function to reduce duplication:
template <typename T>
static int float_significand_size() { return std::numeric_limits<T>::digits - 1; }
include/fmt/format-inl.h
Outdated
| enable_if_t<(sizeof(Double) == sizeof(uint64_t)), int>> | ||
| FMT_API bool grisu_format(Double value, buffer<char>& buf, int precision, | ||
| unsigned options, int& exp) { | ||
| FMT_API bool grisu_format(Double value, size_t sizeof_value, buffer<char>& buf, |
There was a problem hiding this comment.
Since sizeof_value is only used to pass one bit of information (whether argument is float or double) I suggest folding it into options.
d6a0654 to
a887908
Compare
|
Looks good but let me double check the logic in |
orivej
left a comment
There was a problem hiding this comment.
OK. I have left some notes.
|
|
||
| void compute_float_boundaries(fp& lower, fp& upper) const { | ||
| constexpr int min_normal_e = std::numeric_limits<float>::min_exponent - | ||
| std::numeric_limits<double>::digits; |
There was a problem hiding this comment.
The smallest positive normal float is 2^-126. fp(2^-126) is fp(2^52, -178). float min_exponent is -125, double digits is 53: both of these numbers are off by one from what we need, but these 1s cancel each other out.
| std::numeric_limits<double>::digits; | ||
| significand_type half_ulp = 1 << (std::numeric_limits<double>::digits - | ||
| std::numeric_limits<float>::digits - 1); | ||
| if (min_normal_e > e) half_ulp <<= min_normal_e - e; |
There was a problem hiding this comment.
When the float is normal, half_ulp is an obvious constant. When the float is subnormal, the cast to double shifts the significand to the left and effectively decreases the exponent by the number of shifted bits. Then we need to shift half_ulp to the left by the same amount.
| std::numeric_limits<float>::digits - 1); | ||
| if (min_normal_e > e) half_ulp <<= min_normal_e - e; | ||
| upper = normalize<0>(fp(f + half_ulp, e)); | ||
| lower = fp(f - (half_ulp >> (f == implicit_bit && e > min_normal_e)), e); |
There was a problem hiding this comment.
The condition is e > min_normal_e, not e >= min_normal_e, because the smallest positive normal float is the average of its neighbors. (The same is true for the smallest positive normal double but compute_boundaries does not account for that which may be either a bug or irrelevant.)
There was a problem hiding this comment.
The same is true for the smallest positive normal double but compute_boundaries does not account for that which may be either a bug or irrelevant.
Great catch! It's definitely a bug which fortunately doesn't affect the output. Will be fixed in eb879a6.
|
Thanks for the detailed explanation, merged. |
Depends on #1360
Fixes #1336