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

Make double roundtrip when serializing and deserializing to JSON #285

Merged
merged 2 commits into from
May 18, 2016
Merged

Make double roundtrip when serializing and deserializing to JSON #285

merged 2 commits into from
May 18, 2016

Conversation

m7thon
Copy link
Contributor

@m7thon m7thon commented May 17, 2016

The current develop branch of cereal has several problems with serialization and deserialization of double values:

  1. Values are truncated at 17 decimal digits (this is not the same as having 17 decimal digits of precision). For example, 1e-20 is serialized as 0.0. The setting to avoid truncation is given by rapidjson::Writer<>::kDefaultMaxDecimalPlaces and is equal to 324 decimal placed (not 17). I think you may have misunderstood the SetMaxDecimalPlaces() option of the current rapidjson writer.
  2. The special values nan, inf and -inf are not serialized by the current version of rapidjson (nothing is printed at all, resulting in an invalid json representation that cannot be deserialized, i.e., crashes on deserialization).
  3. With the current settings (without rapidjson::kParseFullPrecisionFlag), the deserialization loses up to 3 bits of precision. For example, deserializing 0.9868011474609375 becomes 0.9868011474609376.

This pull request presents a possible fix to all of these issues (1 and 3 being rather trivial changes to json.hpp)

By the way, note that the version of rapidjson you currently include in the develop branch is not 1.0.2, but a rather recent version from the master branch.

@AzothAmmo AzothAmmo added this to the v1.2.0 milestone May 17, 2016
@AzothAmmo
Copy link
Contributor

I'll merge this shortly. Have you considered submitting e5911ad as a PR to rapidjson as well? I still need to modify rapidjson to play nicely with the MSVC warnings and will likely see if I can get that merged upstream as well.

@AzothAmmo AzothAmmo merged commit e5911ad into USCiLab:develop May 18, 2016
@m7thon
Copy link
Contributor Author

m7thon commented May 22, 2016

I submitted a modified PR to rapidjson that adds the functionality of writing/parsing "NaN", "Inf", and "-Inf". This plays more nicely with other parsing options of rapidjson.

@m7thon
Copy link
Contributor Author

m7thon commented May 24, 2016

Ok, so writing and reading of nan/inf/-inf as "NaN", "Inf" and "-Inf" is now possible with rapidjson as an option.

Use by substituting rapidjson::PrettyWriter<WriteStream> with rapidjson::PrettyWriter<WriteStream, rapidjson::UTF8<>, rapidjson::UTF8<>, rapidjson::CrtAllocator, rapidjson::kWriteNanAndInfFlag> and the call itsDocument.ParseStream<rapidjson::kParseFullPrecisionFlag>(itsReadStream); with itsDocument.ParseStream<rapidjson::kParseFullPrecisionFlag | rapidjson::kParseNanAndInfFlag>(itsReadStream);

Alternatively, one can also change the default setting by macros. Before including rapidjson, define:

#define CEREAL_RAPIDJSON_WRITE_DEFAULT_FLAGS kWriteNanAndInfFlag
#define CEREAL_RAPIDJSON_PARSE_DEFAULT_FLAGS kParseFullPrecisionFlag | kParseNanAndInfFlag

Then, don't specify any flags for reading and parsing.

AzothAmmo added a commit that referenced this pull request Aug 5, 2016
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.

2 participants