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

Represent (u)int64 fields with Buffers #16

Closed
wants to merge 1 commit into from
Closed

Represent (u)int64 fields with Buffers #16

wants to merge 1 commit into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Sep 23, 2012

This library currently tries to represent 64-bit signed and unsigned integers as Numbers, which leads to precision problems. I've changed the behavior to parse them into Buffers, and accept Buffers when serializing. The serialization code matches the old behavior when not passed a Buffer.

@chrisdew
Copy link
Owner

This pull request breaks the existing functionality of turning Int64s into JavaScript Numbers. I agree that the conversion can lose data, if the numbers are bigger than 1<<56 iirc.

For this, and #7, we need to add some parameterisation (at import time?) of conversions.

@seishun
Copy link
Contributor Author

seishun commented Oct 15, 2012

It's extremely unlikely anyone is using this module to parse (u)int64s, as they would quickly realize it's unreliable and either give up on it or submit an issue. There's no point in preserving a broken functionality.

If someone really wants a Number, then can always do:

buf.readInt32LE(0) + (buf.readInt32LE(4) << 32)

@chrisdew
Copy link
Owner

No, we need a general solution for 'formatting'. I use int64s as numbers (with bounds checking). They never even exceed 2^32, but they're encoded as int64s in the PB format :-(

@trevex
Copy link
Contributor

trevex commented Oct 25, 2012

An interesting approach is used for longs here (originally from google closure lib). They basically splited the long in two 32bit representations... Unfortunately I currently don't have the time to dig deeper in protobuf and the implementation right now...

@seishun
Copy link
Contributor Author

seishun commented Oct 25, 2012

I've thought about this approach, however, creating and accessing an arbitrary JS object from C++ is quite tricky. Besides, one could easily get the low and high parts by using readInt32LE(0) and readInt32LE(4).

@chrisdew
Copy link
Owner

obsoleted by #19

@chrisdew chrisdew closed this Oct 29, 2012
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.

3 participants