Skip to content

Issue with 64-bit integers on 86_x64 Linux #509

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

Closed
exoticDFT opened this issue Jul 27, 2016 · 11 comments
Closed

Issue with 64-bit integers on 86_x64 Linux #509

exoticDFT opened this issue Jul 27, 2016 · 11 comments

Comments

@exoticDFT
Copy link

There is a problem trying to use the 64-bit integer options on x86_64 Linux versions as the json/config.h file defines Int64 as "long long int". However, x86_64 Linux uses only "long int" to define 64-bit integers. An option should probably be added to the define section when it detects a Linux system to be x86_64. The Int64 typedef should be set as "long int" instead. The UInt64 should follow the same pattern.

@cdunn2001
Copy link
Contributor

Seems reasonable. long long int is 128-bit now, yes? Please submit a pull-request.

@knedlsepp
Copy link

knedlsepp commented Aug 8, 2016

Better use int64_t or the likes if you want exact guarantees about bit-lengths.

@exoticDFT
Copy link
Author

I was thinking the int64_t or uint64_t would be a good route to go. I haven't had a chance to seriously look into fixing this yet, but I have made a fork and will create a pull-request once I have something that works. Thanks for looking into the issue.

@cdunn2001
Copy link
Contributor

Please let me know if you spot a problem, so we can revert this quickly.

@exoticDFT
Copy link
Author

Thanks for fixing this issue. This seems to fix the issue we were having in our CentOS 7 build. However, I was playing around with this myself and I believe it might be a slightly better option to include the int64_t and uint64_t from the cstdint library and add this definition for all platforms. In other words, if you add #include <cstdint> and remove the Microsoft specific __int64 to also use int64_t, etc. this may help keep cross-platform development issues from happening in the future. I have tested this option in both Linux and Windows and it seems to work well.

@cdunn2001
Copy link
Contributor

I'm afraid that might depend on the specific version of VisualStudio. Actually, we've tended to drop support for older compilers with the 1.y.z branch, but I still hesitate to change such a thing since I hate to break anyone.

@cdunn2001
Copy link
Contributor

cdunn2001 commented Sep 28, 2016

This broke binary-compatibility! I'm discussing this with O/S vendors now. Since the damage is done, and since the old version was actually wrong, we might keep it. Or we might have to revert this. Not sure yet...

@cdunn2001 cdunn2001 reopened this Sep 28, 2016
@cdunn2001
Copy link
Contributor

We might also need to #include <cstdint> as suggested by @exoticDFT. See #537.

@sedapsfognik
Copy link

sedapsfognik commented Apr 26, 2017

Hello, guys! I also faced this problem a long time ago.
My fix was to use int64_t and uint64_t as described before.
NOTE: if you edit those typedefs in config.h by your own, you also need to rebuild library by your own.

I think you should not worry about using these typedefs in VS version also, as they are part of C++ standard now.

cdunn2001, you mentioned broken binary-compatibility. What you are talking about? I'm using this fixed version more than a year. But I really want this fix became a part of jsoncpp library.
It is not good to just download and install other C++ libraries, but make several hacks to jsoncpp.

Also, it is totally crazy to install/unpack jsoncpp on linux to /usr/include/?jsoncpp?/json as all samples are referring to just including <json/json.h>. I found a lot of questions from less experienced programmers and the best they achieved is including like <jsoncpp/json/json.h>.

Thanks! Hope, together we can make it better! :)

@cdunn2001
Copy link
Contributor

cdunn2001, you mentioned broken binary-compatibility. What you are talking about?

Well, this:

NOTE: if you edit those typedefs in config.h by your own, you also need to rebuild library by your own.

If the sizes change, then the headers are not binary-compatible with existing builds. I'm not sure how to deal with that. Since we're versioning now, maybe it's ok. But I'm not sure.

There are faster and more C++-14-compliant JSON libraries. The main reasons to use JsonCpp are flexibility and back-compatibility.

I'm not opposed to any particular change though. Submit a pull-request and see who wants to discuss it.

@baylesj
Copy link
Contributor

baylesj commented Jul 1, 2019

Fixed by change to int64_t and uint64_t described by @sedapsfognik above.

@baylesj baylesj closed this as completed Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants