std: correct rounding in parse_hex_float.zig #10743
Conversation
|
Rounding is done according to this presentation where only when the guard bit is non-zero we are going to round up. |
lib/std/fmt/parse_hex_float.zig
Outdated
| @@ -341,7 +341,6 @@ test "f128" { | |||
| // // Min denormalized value. | |||
| .{ .s = "0x1p-16494", .v = math.f128_true_min }, | |||
| .{ .s = "-0x1p-16494", .v = -math.f128_true_min }, | |||
| .{ .s = "0x1.edcb34a235253948765432134674fp-1", .v = 0x1.edcb34a235253948765432134674fp-1 }, | |||
There was a problem hiding this comment.
I wouldn't consider this a duplicate. This case is not covered by the tests in this file. The test for the self-hosted compiler does not count; it's testing something different and it's not part of the standard library tests.
There was a problem hiding this comment.
Reverted in db4c4cf
lib/std/fmt/parse_hex_float.zig
Outdated
| mantissa >>= 2; | ||
| var guardBit = mantissa & 1 == 1; |
There was a problem hiding this comment.
This is equivalent to mantissa & true which is just going to give you a compile error.
What are you trying to do?
There was a problem hiding this comment.
Get the last bit of mantissa as bool.
There was a problem hiding this comment.
Would something like this be considered more in zig ethos?
| var guardBit = mantissa & 1 == 1; | |
| var guardBit = @bitCast(bool, @truncate(u1, mantissa)); |
There was a problem hiding this comment.
Ah I think I just got confused by order of operations, my mistake. Can you just put parens on it?
Related: #114
There was a problem hiding this comment.
Or you could do this. I don't really have a preference.
| var guardBit = mantissa & 1 == 1; | |
| const guard_bit = @truncate(u1, mantissa) != 0; |
lib/std/fmt/parse_hex_float.zig
Outdated
| // - we've truncated more than 0.5ULP (R=S=1) | ||
| // - we've truncated exactly 0.5ULP (R=1 S=0) | ||
| // Were are going to increase the mantissa (round up) | ||
| var exactly_half = (mantissa & 0b11) == 0b10; |
There was a problem hiding this comment.
seems like some of these should be const?
| // - We've truncated exactly 0.5ULP (R=1 S=0), increase the mantissa if the | ||
| // result is odd (G=1). | ||
| // The two checks can be neatly folded as follows. | ||
| mantissa |= @boolToInt(mantissa & 0b100 != 0); |
There was a problem hiding this comment.
Changing this line to:
mantissa += @boolToInt(mantissa & 0b100 != 0);
seems to fix the issue without needing to check both sides of the truncation.
There was a problem hiding this comment.
and then removing the next line: mantissa += 1
There was a problem hiding this comment.
I did a cleanup commit + merge. If you have an additional improvement you would like to make, would you mind submitting it as a follow-up PR?
* fold a couple separate operations into one * use const instead of var * naming conventions
|
Thanks @m-radomski! |
std: correct rounding in parse_hex_float.zig
Addressing #10737, corrected the rounding logic in float128 parsing. Using golang logic (original source as the comment says) no longer produces the off-by-one error.