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 issue #380: Signed integer overflow check #390

Merged
merged 3 commits into from
Dec 13, 2016
Merged

Fix issue #380: Signed integer overflow check #390

merged 3 commits into from
Dec 13, 2016

Conversation

qwename
Copy link
Contributor

@qwename qwename commented Dec 10, 2016

See issue #380.

@qwename
Copy link
Contributor Author

qwename commented Dec 10, 2016

Previously, json::parse("166020696663385964490").dump() gave 18446744073709551562. Now it gives 1.66020696663386e+20.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3bd667c on qwename:integer-overflow into bc28942 on nlohmann:develop.

@qwename
Copy link
Contributor Author

qwename commented Dec 10, 2016

Off-topic, but I had to compile the latest re2c (0.16) as Fedora 25 only provided 0.14.3, which didn't have the -W option. The option was added in 0.15, so this might be useful to add in .github/CONTRIBUTING.md. See http://re2c.org/news/changelog/changelog.html.

It seems like the new version also reduced the generated code length.

EDIT: Now that I look at the generated re2c code again, it seems like the only changes are in minimizing whitespace.

@TurpentineDistillery
Copy link

Or, one could make changes to the .hpp, and then have a pre-commit hook do
git checkout src/json.hpp.re2c && git diff src/json.hpp | patch src/json.hpp.re2c

@nlohmann
Copy link
Owner

@qwename Thanks for the hint on the re2c version. I shall add a note to the contributing file. For the whitespace, you may want to execute make pretty which uses astyle.

@qwename
Copy link
Contributor Author

qwename commented Dec 10, 2016

I missed the part about make pretty, fixed it now.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 92cfc94 on qwename:integer-overflow into bc28942 on nlohmann:develop.

nlohmann added a commit that referenced this pull request Dec 11, 2016
@nlohmann
Copy link
Owner

There are some conversion warnings. Can you fix them?

Yixin Zhang added 3 commits December 12, 2016 21:12
Instead of checking something like `x * y + z > max` where `x * y` can
overflow, check for `x > (max - z) / y` instead.
Use static_cast on digit.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 703d4ba on qwename:integer-overflow into 79fa8b2 on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 703d4ba on qwename:integer-overflow into 79fa8b2 on nlohmann:develop.

@nlohmann
Copy link
Owner

Thanks!

@nlohmann nlohmann self-assigned this Dec 13, 2016
@nlohmann nlohmann added this to the Release 2.0.9 milestone Dec 13, 2016
@nlohmann nlohmann merged commit 4e2fb1a into nlohmann:develop Dec 13, 2016
@nlohmann
Copy link
Owner

Thanks! I shall add a regression test for 166020696663385964490.

nlohmann added a commit that referenced this pull request Dec 13, 2016
@qwename qwename deleted the integer-overflow branch December 14, 2016 01:50
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.

4 participants