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

Current rapidjson #221

Closed
wants to merge 5 commits into from
Closed

Current rapidjson #221

wants to merge 5 commits into from

Conversation

m7thon
Copy link
Contributor

@m7thon m7thon commented Aug 20, 2015

EDIT Feb 17, 2016 (update to newest cereal and rapidjson):

This updates rapidjson to the current master and discards the modifications made by cereal. The differences to the previously included version of rapidjson are, as far as I can tell:

  • The precision for double output has a different meaning. Currently rapidjson supports "truncated" double output, e.g., a precision of 2 means: 0.1285 -> 0.12, 0.004 -> 0.0, 1.23 e-7 -> 0.0.
  • I modified the current rapidjson to support writing and parsing of nan, inf and -inf doubles. This is a non-standard json extension that rapidjson currently does not support (but may in the future). Note that the ability to parsing -inf seems to be a new feature for cereal.
  • long double, long long, and unsigned long long are no longer handled in rapidjson. However, it seems this is now handled by cereal anyway by writing these types as strings.

The file json.hpp is modified only minimally to work with the current rapidjson. Importantly, the parser is given the rapidjson::kParseFullPrecisionFlag, which means that double values are parsed in full precision (AKA correctly). This ensures that the roundtrip C++ -> json -> C++ works for all doubles.

@AzothAmmo
Copy link
Contributor

I'll look at this in the near future since I have a little more time for cereal right now - looks like automated tests are failing but I would sort this out in a merge. This was on my list of things to do (see #82) so if this looks good and integrates well that'd be a great help.

@m7thon
Copy link
Contributor Author

m7thon commented Aug 20, 2015

Compilation just gives one harmless -Wshadow error in rapidjson/reader.h line 752. You'll have to see how you want to deal with it.

Disabling -Werror as I have done above is probably not what you want to do ;-)

@m7thon
Copy link
Contributor Author

m7thon commented Aug 20, 2015

By the way, it seems your CI build doesn't actually run your unit tests, just compiles them. Is that intended?

@AzothAmmo
Copy link
Contributor

They get run after compilation.

-----Original Message-----
From: "Michael Thon" notifications@github.com
Sent: ‎8/‎20/‎2015 4:45 PM
To: "USCiLab/cereal" cereal@noreply.github.com
Cc: "Shane Grant" w.shane.grant@gmail.com
Subject: Re: [cereal] Current rapidjson (#221)

By the way, it seems your CI build doesn't actually run your unit tests, just compiles them. Is that intended?

Reply to this email directly or view it on GitHub.

@bagobor
Copy link

bagobor commented Aug 29, 2015

'rapidjson/filewritestream.h' - file is absent in latest rapidjson packege.

@m7thon
Copy link
Contributor Author

m7thon commented Aug 30, 2015

@bagobor Do you mean filestream.h ? This has been replaced by filereadstream.h and filewritestream.h, and I think these now work slightly differently. You can have a look at the rapidjson docs here.

But this (filestream.h) was not used by cereal anyway.

@bagobor
Copy link

bagobor commented Aug 30, 2015

@m7thon I'm very sorry ( was incorrect 'paste'), I meant file 'rapidjson/genericstream.h'

@m7thon
Copy link
Contributor Author

m7thon commented Aug 30, 2015

Ah yes. But you can use the old file. I updated it to match the example given in the current rapidjson documentation, and use the RAPIDJSON... namespace macros. See my first two commits.

EDIT: In the newest version, use 'istreamwrapper.h' and 'ostreamwrapper.h' instead.

@m7thon m7thon closed this May 17, 2016
@m7thon m7thon deleted the current-rapidjson-and-json-improvements branch May 17, 2016 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants