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 strtod #1290

Merged
merged 25 commits into from
Jun 17, 2018
Merged

Fix strtod #1290

merged 25 commits into from
Jun 17, 2018

Conversation

abolz
Copy link
Contributor

@abolz abolz commented Jun 15, 2018

Fixes #849, #1249, #1251, #1253, #1256, #1259

The issues were mainly because of unsigned integer overflow and boundary cases. Switched to signed integer arithmetic.

To do:

  • This patch currently requires "small inputs" and "small exponents" due to integer overflow. Fixing this probably requires some more changes in ParseNumber. (Or simply using int64 instead of int32)
  • Some inaccuracies in full-precision mode (due to discarding non-zero bits) should be fixed.

@miloyip
Copy link
Collaborator

miloyip commented Jun 15, 2018

Really thank for your contribution.
There is some valgrind errors such as https://travis-ci.org/Tencent/rapidjson/jobs/392709049#L2555 Can you take a look?

@abolz
Copy link
Contributor Author

abolz commented Jun 15, 2018

Yes, looking into it. But honestly I have no idea whats wrong... Valgrind complains about an uninitialized variable of size 4 in StrtodBigInteger.

@abolz
Copy link
Contributor Author

abolz commented Jun 15, 2018

BTW could you cancel all the running appveyor builds?

@coveralls
Copy link

coveralls commented Jun 16, 2018

Coverage Status

Coverage increased (+0.0003%) to 99.921% when pulling 7101911 on abolz:fix-strtod into 01c7174 on Tencent:master.

@abolz
Copy link
Contributor Author

abolz commented Jun 16, 2018

The valgrind error is fixed now (was due to an incorrect offset computation in BigInteger operator<<).
There are still some tests for edge cases in full-precision mode failing, which need to be fixed.

until I know what the normal-precision algorithm really does...
@miloyip miloyip merged commit 6cc3910 into Tencent:master Jun 17, 2018
@miloyip
Copy link
Collaborator

miloyip commented Jun 17, 2018

Thank you for your precious work!

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.

Inconsistent reporting of numbers that are too big to be stored in a double
3 participants