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

Floating-point formatting (#360) #866

Closed
wants to merge 6 commits into from
Closed

Floating-point formatting (#360) #866

wants to merge 6 commits into from

Conversation

abolz
Copy link
Contributor

@abolz abolz commented Dec 9, 2017

Try to fix #360. printf doesn't have a "to shortest" mode and std::to_chars is not available yet. Use the Grisu2 algorithm for floating-point formatting which is a nice compromise between using printf with a precision of digits10 and max_digits10, i.e. the resulting decimal representation is guaranteed to round-trip and is almost always the shortest such representation.

This PR is mostly to see if everything compiles fine and check that all tests are passing. The change is quite large. But maybe it serves as a starting point for an alternative to always use max_digits10.

This is an attempt to fix #360. The algorithm produces
decimal representations which are guaranteed to roundtrip
and in ~99.8% actually produces the shortest possible
representation. So this is a nice compromise between using
a precision of digits10 and max_digits10.

Note 1:

The implementation only works for IEEE single/double precision
numbers. So the old implementation is kept for compatibility
with non-IEEE implementations and 'long double'.

Note 2:

If number_float_t is 'float', not all serialized numbers can
be recovered using strtod (strtof works, though). (There is
exactly one such number and the result is off by 1 ulp.)
This can be avoided by changing the implementation (the fix
is trivial), but then the resulting decimal numbers are not
exactly short.

Note 3:

The code should be replaced by std::to_chars once it is available.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 38e678d on abolz:dtoa into 0693945 on nlohmann:develop.

@nlohmann
Copy link
Owner

nlohmann commented Dec 9, 2017

What is the license of the included conversion code?

@abolz
Copy link
Contributor Author

abolz commented Dec 9, 2017

The conversion code is based on the reference implementation by Florian Loitsch which is distributed under the MIT license. I added a NOTICE file containing the license but obviously missed to commit it. Is it ok to add a NOTICE file? Or add the license to then end of json.hpp?

@nlohmann
Copy link
Owner

Thanks a lot for the PR. I am currently a bit torn about merging it for the 3.0.0 release, because it involves quite a lot code and I won't have time for a thorough review until then. Furthermore, it could benefit from #700 as we can move this code in a separate (development) header. On the other hand, it does change the behavior of the library (albeit for the better), so maybe that adding it after 3.0.0 would mean to quickly release a 4.0.0 version.

Any thoughts on this?

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Dec 10, 2017
@gregmarr
Copy link
Contributor

I would say that this is a bug fix not affecting the public API, and thus fixing it in a point release would be fine under SemVer guidelines.
I agree that adding it after #700 would allow all of this code to be in a separate header, and thus be properly identified with its license.

@abolz
Copy link
Contributor Author

abolz commented Dec 11, 2017

I would even say that that using max_digits10 for formatting is the bug fix and then this PR would only be a small enhancement. (And I could actually live without it.)
I agree with both of you that adding/reconsidering this PR after #700 would be much nicer.

@stale
Copy link

stale bot commented Jan 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jan 10, 2018
@nlohmann nlohmann removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jan 14, 2018
@abolz
Copy link
Contributor Author

abolz commented Jan 15, 2018

Is there still some interest in this PR?

I'm asking because I tried to rebase this PR and failed miserably. Two times... :-) So if there is any interest, I'd like to close this and open a new PR (which seems a lot easier for me...)

@nlohmann
Copy link
Owner

Yes, please! Now that the header is split, we can focus on adding such features.

@abolz
Copy link
Contributor Author

abolz commented Jan 15, 2018

Ok. Where shall I put the implementation? I thought about something like a to_chars.hpp file in detail/conversions.

@nlohmann
Copy link
Owner

Sounds good.

@abolz abolz closed this Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: please discuss please discuss the issue or vote for your favorite option
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loss of precision when serializing <double>
4 participants