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 an integral float overflow for very large strings #243

Merged
merged 1 commit into from
Feb 16, 2016

Conversation

johnynek
Copy link
Collaborator

we hit this with a 70MB string. Work around by removing a byte. Better to fix.

@@ -53,7 +53,7 @@ trait StringInjections extends NumericInjections {
val decBuf = decRef.get
val dec = decBuf._1
val buf = decBuf._2
val maxSpace = (b.length * dec.maxCharsPerByte).toInt + 1
val maxSpace = (b.length * (dec.maxCharsPerByte.toDouble)).toInt + 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put a comment, that for cases where it's possible to have 2B, just try using 2B. I'd also drop the () and maybe just toDouble on the length, since that's the thing that can be large.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, the bug is that dec.maxCharsPerByte returns a Float. That can only store all integers up to 2^{24}. After that it steps up by 2. So the toInt truncates by 2 in that case, not rounding down by one.

We need to convert that Float to a Double before the *.

I don't know what 2B means in this case. Can you explain?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2B = 2 billion (1 << 31), since that's how toInt handles larger values.

But yeah, Float -> Double makes more sense than integral -> Double, so that seems fine.

ianoc added a commit that referenced this pull request Feb 16, 2016
Fix an integral float overflow for very large strings
@ianoc ianoc merged commit 73de35f into develop Feb 16, 2016
@ianoc ianoc deleted the oscar/fix-float-overflow branch February 16, 2016 19:45
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.

None yet

4 participants