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

i64 implementation should be optional #1

Open
roblabla opened this issue Mar 27, 2016 · 13 comments
Open

i64 implementation should be optional #1

roblabla opened this issue Mar 27, 2016 · 13 comments

Comments

@roblabla
Copy link
Contributor

i64 cannot be represented "properly" in many languages (cough_JS_cough). https://github.com/rom1504/ProtoDef/blob/master/doc/datatypes.md#numeric we specify that an example value is [0, 1].

My belief is that the default ProtoDef implementation should only specify "common" ranges iu. Our current i64 in NMP is just an [u32, u32] anyway.

@rom1504
Copy link
Member

rom1504 commented Apr 2, 2016

well most languages can handle long fine (for example elixir can). It's just js that can't do it.
but yeah having [0,1] in the json tests (and doc) is confusing, might be better to put the actual value in the json since json support any-length integers. (but then got to figure out how to parse the value with JSON.parse, might be possible, I think there's some option to parse fields differently)

@roblabla
Copy link
Contributor Author

roblabla commented Apr 2, 2016

Python can't read i64 (I think, need to check)
Standard lua can't read i64.

That's already 3 languages that can't properly support iu64 without some kind of external library (that would likely kill perf, though that needs to be measured).

I do agree that iu64 are useful though (if only because mc uses it quite a lot). Which is why I think iu64 should be implementation-defined. More or less, the spec should say "hey, iu64 should exist on your platform, however its output may vary depending on platform support. As such, we don't provide tests for it."

@hansihe
Copy link
Member

hansihe commented Apr 2, 2016

Python supports arbitrarily large integers.

If 64 bit numeric types are made optional, and you then want to write a protocol definition that uses 64 bit integers and works across multiple languages, you would then have to use two 32 bit datatypes and assemble them manually after the fact. It seems to me like it would be a better approach to let the protodef implementation handle this in the best possible way for each language.

@hansihe
Copy link
Member

hansihe commented Apr 2, 2016

One way to do tests for 64 bit integers would be to supply the values as strings, then do the comparison in the proper way for the implementation. Not a very nice way of doing it though.

@roblabla
Copy link
Contributor Author

roblabla commented Apr 2, 2016

Yeah, that's what I mean by implementation-defined. Some impl might return a 2-elem array, others a number.

The bigger problem comes when you want to use the i64 field in a switch, an array count, etc... Since it is impl-defined, there is no reliable way to know if it's safe to use it or not.

I guess we could use a hex string for the test. That would be the simplest to implement for everyone (parse the string into a single number, or split it in two and parse the string into two numbers).

@rom1504
Copy link
Member

rom1504 commented Apr 6, 2016

I think we could just write the base-10 representation in the json tests and use this package https://www.npmjs.com/package/long#longfromstringstr-unsigned-radix

edit: ah but I see, that would mean having to convert the value to a 2 element array in arrays/ objects/... not quite convenient

maybe https://github.com/sidorares/json-bigint could help

edit2: well what we are doing currently is saying "a 2 element array is the standard way of representing a long". For us in node-protodef that means no conversion. In elixir-protodef it means converting.
We could just say "the long is represented as a base-10 number in the test json" which would make more sense and it would be node-protodef that would have to convert because it doesn't have long handling.

@hansihe
Copy link
Member

hansihe commented Apr 6, 2016

I can't comment on how easy it is to make it work in node, but from a spec perspective it would certainly make the most sense to use the standard json number format.

@roblabla
Copy link
Contributor Author

roblabla commented Apr 7, 2016

The problem is that it is impossible to efficiently implement i64 in many languages. Even if in the JSON, the number is 64-bits (Did you try putting something bigger than 2^53 (the largest consecutive int a double can store) to make sure it actually worked ? Try to put 9007199254740993 in the JSON, I'd be curious if elixir actually parsed it correctly. Most JSON.parse impl I've seen parse the num as a double, meaning they'll return 9007199254740992. http://stackoverflow.com/a/1848762

And creating our own JSON parser isn't fun.

@rom1504
Copy link
Member

rom1504 commented Apr 7, 2016

https://github.com/sidorares/json-bigint can parse infinitely big numbers in json ;)

@rom1504
Copy link
Member

rom1504 commented Apr 7, 2016

The problem is that it is impossible to efficiently implement i64 in many languages.

Most of the protocols we are currently targetting do use i64 (and some other ones do too).
I think we should support common types, not only good types. Supporting only good types is what ProtoBuf do. ProtoDef should be able to handle bad but common types too I think.

@hansihe
Copy link
Member

hansihe commented Apr 7, 2016

Elixir does indeed parse it successfully.

iex(1)> Poison.Parser.parse!("9007199254740993")
9007199254740993
iex(2)> Poison.Parser.parse!("9823748762756238745298737657236458972344")
9823748762756238745298737657236458972344

I don't think not supporting i64 is as common as you think. The only two languages I can think of that can't are Javascript and Lua.

Languages like TCL, Python, Ruby, and even PHP seems to support them.

@roblabla
Copy link
Contributor Author

roblabla commented Apr 7, 2016

Fine. I guess I lose this one.

We should specify that the output should be a number though, not a 2-elem array. And figure a way to implement it in Node so we can use an i64 result in a switch clause.

@roblabla
Copy link
Contributor Author

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

No branches or pull requests

3 participants