Ignore zero-padding for non-finite floating points - #2310
Conversation
| if (std::signbit(-nan)) { | ||
| EXPECT_EQ("-nan", fmt::format("{}", -nan)); | ||
| else | ||
| EXPECT_EQ("-nan", fmt::format("{:+06}", -nan)); |
There was a problem hiding this comment.
This should be " -nan" i.e. the width should still apply. Same elsewhere.
| return write_padded(out, specs, size, [=](reserve_iterator<OutputIt> it) { | ||
| auto copy_it = [=](reserve_iterator<OutputIt> it) { | ||
| if (sign) *it++ = static_cast<Char>(data::signs[sign]); | ||
| return copy_str<Char>(str, str + str_size, it); | ||
| }); | ||
| }; | ||
| // no '0'-padding applied for non-finite values | ||
| const bool is_zero_fill = | ||
| specs.fill.size() == 1 && *specs.fill.data() == static_cast<Char>('0'); | ||
| return is_zero_fill ? base_iterator(out, copy_it(reserve(out, size))) | ||
| : write_padded<align::right>(out, specs, size, copy_it); |
There was a problem hiding this comment.
I suggest copying specs and changing fill from 0 to (space) instead.
| // '0'-fill option sets alignment to numeric overwriting any user-provided | ||
| // alignment |
There was a problem hiding this comment.
Is this the desired behavior? Currently we lose the originally provided alignment because it seems to get replaced by align::numeric when using 0-padding.
There was a problem hiding this comment.
No, we should preserve the original alignment. I think it can be dome by changing
Line 1950 in ca46637
to something like
if (specs_.align == align::none) specs_.align = align::numeric; There was a problem hiding this comment.
Thanks, that works!
vitaut
left a comment
vitaut
left a comment
There was a problem hiding this comment.
Two nits, otherwise LGTM.
| constexpr size_t str_size = 3; | ||
| auto sign = fspecs.sign; | ||
| auto size = str_size + (sign ? 1 : 0); | ||
| // replace '0'-padding with space for non-finite values |
There was a problem hiding this comment.
nit: please make this a proper sentence:
// Replace '0'-padding with space for non-finite values.| else | ||
| EXPECT_EQ(" -nan", fmt::format("{:+06}", -nan)); | ||
| } else | ||
| fmt::print("Warning: compiler doesn't handle negative NaN correctly"); |
There was a problem hiding this comment.
nit: please wrap the else statement in {} for consistency with if:
} else {
fmt::print("Warning: compiler doesn't handle negative NaN correctly");
}
|
Thank you! |
2 participants
Fixes #2305.
Is there a better way to check for the
0-padding-option than what I wrote?