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

Fixed Issue #186 - add strto(f|d|ld) overload wrappers, "-0.0" special case and FP trailing zero #191

Merged
merged 8 commits into from
Jan 24, 2016

Conversation

twelsby
Copy link
Contributor

@twelsby twelsby commented Jan 24, 2016

This pull request contains the following changes that have been discussed in issues #186 and #187:

  1. get_number() now calls overload wrappers for strtof(), strtod() and strtold(), instead of callingstrtold()directly. This prevents the double rounding that causes errors in particular cases wherenumber_float_tis not along double` due to the cast that is then required.
  2. parse_internal() now tests for the special case of -0.0 and ensures that such numbers are stored as a float as they cannot be accurately represented with integer types.
  3. dump() now tests for floating point numbers that do not have a fractional component (e.g. 1.0, 42.0, -36.0) and in this case uses a std::fixed and std:setprecision(1) representation. This is necessary for certain round trip tests to pass successfully.

@twelsby twelsby changed the title Fix Issue #186 - add strto(f|d|ld) overload wrappers, "-0.0" special case and FP trailing zero Fixed Issue #186 - add strto(f|d|ld) overload wrappers, "-0.0" special case and FP trailing zero Jan 24, 2016
@nlohmann
Copy link
Owner

I see that the Travis builds still fail and there is some debug output. Could you please drop a note once this is ready to merge?

@nlohmann nlohmann added this to the Release 1.0.1 milestone Jan 24, 2016
@twelsby
Copy link
Contributor Author

twelsby commented Jan 24, 2016

Note that this PR is currently a work in progress. It passes all tests locally including using identical settings as used by Travis for both CLANG 3.7 and GCC 4.9 (as well as GCC 5.2.1) and, obviously, AppVeyor. The only test that fails under Travis is a new test for "1.00000000000000011102230246251565404236316680908203126" which is expected to parse to 1.00000000000000022 but instead parses to exactly 1.

The DEBUG ONLY commit (f79d52b) results shows that it is only the least significant bit that differs. The interesting thing is that doing a simple local test using strtod() under GCC produces the expected result of 1.00000000000000022 however if it is first parsed using strtold() and then cast to a double then the result is exactly 1. This hints that an earlier version of libstdc++ may be being used in Travis that uses an implementation of strtod() that itself calls strtold() and then does the cast. Unfortunately it is not clear what version of libstdc++ Travis is using. This matter is still being investigated but one solution would be simply to only run this particular test under AppVeyor as this implementation still has better compliance than the current master (which currently also fails this test).

@twelsby
Copy link
Contributor Author

twelsby commented Jan 24, 2016

Debug dumps added to pull #193 reveal that the error is definitely coming from strtod() so it cannot be fixed from the library end. The only solution is to exclude that test under GCC/clang until Travis is updated.

@twelsby
Copy link
Contributor Author

twelsby commented Jan 24, 2016

@nlohmann, the issue is now resolved and this is ready for merge.

nlohmann added a commit that referenced this pull request Jan 24, 2016
Fixed Issue #186 - add strto(f|d|ld) overload wrappers, "-0.0" special case and FP trailing zero
@nlohmann nlohmann merged commit ad5d1da into nlohmann:master Jan 24, 2016
@nlohmann
Copy link
Owner

Thanks a lot!!

nlohmann added a commit that referenced this pull request Jan 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants