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

Fix round trip floating point bug on x64 platforms #50

Merged
merged 5 commits into from Apr 10, 2018

Conversation

@PathogenDavid
Copy link
Contributor

commented Apr 9, 2018

Short version: This pull request changes the format string for doubles and floats from R to G17 and G9 respectively as per the Standard Numeric Format Strings documentation. This fixes round-trip failures for doubles on x64 platforms.

This is essentially the same as #10, except for floating-point numbers.

Note that this commit contains the actual fix: PathogenDavid@3d3983a (The other commits only touch the tests.)

The problem

The R format string does not work as expected on x64 platforms.

The issue is discussed in length in dotnet/coreclr#13106 and the rejected PR dotnet/coreclr#13140, and is documented in both the R and G sections of the Standard Numeric Format Strings documentation.


The bug can be observed by checking out PathogenDavid@a0dbebf (which includes a test for the bug, but does not fix it.)

If you run it in x64 mode nunit3-console SharpYaml.Tests.exe, you will get failures.
If you run it in x86 mode nunit3-console SharpYaml.Tests.exe --x86, you will not.

Breaking Change

This change is technically a breaking change (which is why dotnet/coreclr#13140 was eventually rejected.) The deserialized values of previously serialized data should be identical. However, it will likely not serialize to the exact same string.

This is caused by the fact that the R format specifier tries to use a more human-friendly shorter representation where possible, whereas G17 will not. This can be observed on some of the serialization tests which broke as a result of this change: PathogenDavid@151b38a


In addition to fixing this issue, I've added a test that verifies edge-case floating point values round-trip as expected.

@PathogenDavid

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

It is may be worth noting that it is unclear if R is actually broken for System.Single. The documentation states that it is, but there is an open issue dotnet/corefx#26785 requesting examples of values that could cause issues, but no examples have been given. Regardless, the behavior of R is not IEEE-compliant, and only using G for one type and not the other would be inconsistent.

@xoofx

This comment has been minimized.

Copy link
Owner

commented Apr 10, 2018

Ok...hm... the unfortunate side effect of this change is that numbers are no longer simply "readable" in the output...

@xoofx xoofx merged commit a70c780 into xoofx:master Apr 10, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@xoofx

This comment has been minimized.

Copy link
Owner

commented Apr 10, 2018

But thanks! Hope it won't have too much impact on existing users.

@PathogenDavid

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

Yeah, I wasn't crazy about that either, but I feel like round-trip consistency is probably more important for serialization purposes. If it bothers someone enough, it would not be terribly hard to add something to SerializerSettings to allow changing the way floating point numbers are serialized for people who would prefer readability over consistency.

No problem, thanks for the merge!

@xoofx

This comment has been minimized.

Copy link
Owner

commented Apr 10, 2018

Yeah, I wasn't crazy about that either, but I feel like round-trip consistency is probably more important for serialization purposes. If it bothers someone enough, it would not be terribly hard to add something to SerializerSettings to allow changing the way floating point numbers are serialized for people who would prefer readability over consistency.

Indeed, let's see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.