Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible float parsing bug #20275

Closed
MasonRemaley opened this issue Jun 12, 2024 · 0 comments · Fixed by #20287
Closed

Possible float parsing bug #20275

MasonRemaley opened this issue Jun 12, 2024 · 0 comments · Fixed by #20287
Labels
bug Observed behavior contradicts documented or intended behavior standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@MasonRemaley
Copy link
Contributor

MasonRemaley commented Jun 12, 2024

Zig Version

0.14.0-dev.23+d9bd34fd0

Steps to Reproduce and Observed Behavior

The following test fails:

test "float issue" {
    const a: f32 = 0xffffffffffffffff.0p0;
    const b: f32 = try std.fmt.parseFloat(f32, "0xffffffffffffffff.0p0");
    try std.testing.expectEqual(a, b); // failed: expected 1.8446744e19, found 1.1529215e18
}

At first glance it's hard to imagine why this would fail, since the compiler itself is using parseFloat as well. However, the compiler always parses floats as f128s, so here's a more illustrative test case:

test "float issue" {
    const a: f32 = try std.fmt.parseFloat(f32, "0xffffffffffffffff.0p0");
    const b: f32 = @floatCast(try std.fmt.parseFloat(f128, "0xffffffffffffffff.0p0"));
    try std.testing.expectEqual(a, b); // failed: expected 1.1529215e18, found 1.8446744e19
}

If I understand the situation correctly, this should pass:

  • Parsing as a f32 should get the closest possible f32 representation to the given number
  • Parsing as a f128 should get the closest possible f128 to the given number, and then float casting that should round to the closest possible f32 to that number
  • These results should therefore be the same, but they are not for the example value and other very large values

Besides resulting in an incorrect value, this has a minor detrimental effect on #20271--in the interest of being consistent results with Zig, the ZON parser has to parse all floats as f128s even when it knows the destination precision and could technically save time by parsing as f32s.

I suspect that the cause of this is related to either rounding modes or handling of very large/out of range floats, but I'm not sure how to dig in further.

Expected Behavior

I expected parsing floating point literal strings as a given type to produce the same result as including that literal in the source code and ascribing the same type.

@MasonRemaley MasonRemaley added the bug Observed behavior contradicts documented or intended behavior label Jun 12, 2024
@MasonRemaley MasonRemaley mentioned this issue Jun 12, 2024
5 tasks
tiehuis added a commit to tiehuis/zig that referenced this issue Jun 14, 2024
There were two primary issues at play here:
 1. The hex float prefix was not handled correctly when the stream was
    reset for the fallback parsing path, which occured when the mantissa was
    longer max mantissa digits.
 2. The implied exponent was not adjusted for hex-floats in this branch.

Additionally, some of the float parsing routines have been condensed, making
use of comptime.

closes ziglang#20275
ifreund pushed a commit that referenced this issue Jun 15, 2024
There were two primary issues at play here:
 1. The hex float prefix was not handled correctly when the stream was
    reset for the fallback parsing path, which occured when the mantissa was
    longer max mantissa digits.
 2. The implied exponent was not adjusted for hex-floats in this branch.

Additionally, some of the float parsing routines have been condensed, making
use of comptime.

closes #20275
@Vexu Vexu added this to the 0.14.0 milestone Jun 15, 2024
@Vexu Vexu added the standard library This issue involves writing Zig code for the standard library. label Jun 15, 2024
ryoppippi pushed a commit to ryoppippi/zig that referenced this issue Jul 5, 2024
There were two primary issues at play here:
 1. The hex float prefix was not handled correctly when the stream was
    reset for the fallback parsing path, which occured when the mantissa was
    longer max mantissa digits.
 2. The implied exponent was not adjusted for hex-floats in this branch.

Additionally, some of the float parsing routines have been condensed, making
use of comptime.

closes ziglang#20275
SammyJames pushed a commit to SammyJames/zig that referenced this issue Aug 7, 2024
There were two primary issues at play here:
 1. The hex float prefix was not handled correctly when the stream was
    reset for the fallback parsing path, which occured when the mantissa was
    longer max mantissa digits.
 2. The implied exponent was not adjusted for hex-floats in this branch.

Additionally, some of the float parsing routines have been condensed, making
use of comptime.

closes ziglang#20275
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants