dropbox / json11

A tiny JSON library for C++11.
MIT License
2.55k stars 616 forks source link

Json constructors for int64 and uint32 / uint64 types? #16

Open lehni opened 10 years ago

lehni commented 10 years ago

First of all, congrats on this great little library! I've been looking for a lightweight modern implementation for a while now, really happy to finally have found it.

The only thing that I was a bit irritated by was that there isn't any support for int64 and uint32 / 64 types.

Json doesn't make that distinction, but if you want a non-float value that is larger than 32 bits, or an unsigned int without loosing the values from 1 << 15 and beyond, then you currently have to go through double, which seems odd.

The simple solution could be to internally store all such values as long (in a new JsonLong class), not int, and then do the casting in int_value(), long_value() uint_value() (or unsigned_int_value() if you like it verbose), ulong_value(), ...) But I guess this would still cause troubles when exporting, as it will have to be known if a value was signed or unsigned.

skabbes commented 10 years ago

I believe that the reasoning here is that JSON can't actually support anything larger that 53 bit integers. If your use case is timestamps, double has plenty of precision as clarified in this comment

https://github.com/dropbox/json11/blob/710352234426a869b5ebd4f2e5ff8c71ccd6bc07/json11.hpp#L24-L28

Storing as a 32 bit integer (as either signed or unsigned) will be fine, since doubles have more than enough precision to represent this.

uint32_t value = static_cast<uint32_t>( json_thing.number_value() )

will always be safe, but casting to a 64 bit integer type will cause issues with values larger than 53 bits.

So, in my opinion, 64 bit types are a non-goal of this library, and 32 bits types are simply syntactic sugar (which would be argued for either way).

lehni commented 10 years ago

Alright, that all makes sense, thanks for clarifying. The syntactic sugar for unsigned 32 bit could still be nice though?

skabbes commented 10 years ago

I would probably agree, but I'm not the library maintainer :)

skifire1983 commented 8 years ago

@skabbes and @lehni made a pull request https://github.com/dropbox/json11/pull/61 (also signed the SLA).

taozhijiang commented 8 years ago

Yes, I store uint64 session_id, but can not store with the precise. I am wondering whether there are any other methods to improve this.

string str1 = R"({"message_id":105308320612483198,"msg_type":3,"order":0,"ques_id":0,"session_id":105308187502051928,"site_id":122062,"text":"XXXXXXX","timestamp":1471140271,"visitor_id":9941658010949867158,"worker_id":133746})"; string err; auto json = json11::Json::parse(str1, err); cout << (unsigned long long) (json["message_id"].number_value()) << endl;

the value is 105308320612483200 maybe I can ignore some bits....

artwyman commented 8 years ago

If you need fidelity for a full 64 bits, your best bet is to encode your IDs as strings rather than numbers. Or modify your own fork with something like #61 if you can accept the cross-language compatibility concerns discussed there.

taozhijiang commented 8 years ago

@artwyman thanks. I checked, the #61 implement may have some problem, It can not reach int64_max, int64_min, uint64_max limits. I forked and enhanced at https://github.com/taozhijiang/json11

With out the consider like javascript, I think int64_t and uint_64 are very important at most cases. I suggest create another branch may a better solution. ;-)

artwyman commented 8 years ago

Maintaining a separate branch would add some maintenance overhead, though admittedly less confusingly than multiple forks. It might be better to provide both options controlled by ifdefs, or as distinct types in the same library, though I'd be interested to hear @j4cbo's opinion on the matter.

fosterbrereton commented 8 years ago

I understand json11 is limited to 53-bit integers because it might round-trip them through double - but that is a limitation of the implementation, not of the JSON specification. (If I am mistaken here and missed something in the JSON spec, please let me know. Otherwise...)

I think a proper implementation would use intmax_t to store integer values and separate integer storage from double. Support for implicit casting between intmax_t and double should still be available, with the implied understanding that there would be cases of loss in precision (which is already the case, only going from double -> int.)

artwyman commented 8 years ago

@fosterbrereton That's an intentional limitation for compatibility, as described here: https://github.com/dropbox/json11/blob/710352234426a869b5ebd4f2e5ff8c71ccd6bc07/json11.hpp#L16-L22

It's not about the JSON spec (which doesn't say anything about numeric sizes) but about compatibility with all major implementations, including those such as JavaScript which treat all numbers as doubles. We designed json11 for network traffic, and want it to produce and consume JSON which can be exchanged with any other implementation.

fosterbrereton commented 8 years ago

If everything has to be limited to what can round-trip through a double, then, why add all the extra code to support int? The library would be cleaner and more correct with respect to the intent of supporting double for JavaScript's sake.

Separating storage for int and double implies they should be treated differently to take advantage of each's benefits (which would be great). It's no surprise, then, that library users might misinterpret the library's intent and file bug reports and/or feature requests in kind.

rektide commented 7 years ago

It's not about the JSON spec (which doesn't say anything about numeric sizes) but about compatibility with all major implementations, including those such as JavaScript which treat all numbers as doubles.

But JavaScript will implicity convert to double! Ignoring the spec and lowering precision deliberately because one implementation has a funky numerical typing system is really a frustrating resolution to this ticket. Further, in the future JS is expected to have 64 bit ints & uints (https://github.com/tc39/ecmascript_simd &c)! Exercising this nuclear option now will could significantly harm the usability of this library in that future.

uint64_t support would be really really really nice, and it should totally happen. JavaScript will work just fine if json11 does this, it'll just lose precision, like JavaScript does when you are > MAX_SAFE_INTEGER. Which is fine and well known and JS developers (raise hand) know they have to cope with this. Please don't limit this lib artificially.

This would be very useful for implementing https://github.com/PowerDNS/pdns/pull/4915 for example.