Skip to content

Double <-> String round trip fails #98

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
osx2000 opened this issue Dec 19, 2014 · 1 comment · Fixed by #109
Closed

Double <-> String round trip fails #98

osx2000 opened this issue Dec 19, 2014 · 1 comment · Fixed by #109

Comments

@osx2000
Copy link

osx2000 commented Dec 19, 2014

The current double serialization code doesn't work as good as possible for doubles - the round trip double -> string -> double creates different double output from input in a lot of cases.

The reason for this is that the number of digits (16) used to print the double to string is too low.

This http://stackoverflow.com/a/16941784/2520006 links to some background reading.

The fix is quite simple: Just print 17 digits.

The attached test case demonstrates problem and fix.

See also http://sourceforge.net/p/jsoncpp/bugs/63/


#define STANDALONE 1

#include <stdio.h>
#include <string.h>

#define _USE_MATH_DEFINES
#include <math.h>

#include <string>
#include <iostream>
#include <iomanip> 
#include <limits>

namespace {

double pi = M_PI;

double r2 = sqrt(2.0);

const float samples[] = { pi, pi + r2, pi + r2 * 2, pi + r2 * 3, pi + r2 * 4, pi + r2 * 5, pi + r2 * 6, pi + r2 * 7 };

const int samplessize = sizeof(samples)/sizeof(samples[0]);

}


// Code exactly as in jsoncpp amalgamated
/** Change ',' to '.' everywhere in buffer.
 *
 * We had a sophisticated way, but it did not work in WinCE.
 * @see https://github.com/open-source-parsers/jsoncpp/pull/9
 */
static inline void fixNumericLocale(char* begin, char* end) {
  while (begin < end) {
    if (*begin == ',') {
      *begin = '.';
    }
    ++begin;
  }
}

std::string valueToString16(double value) {
  // Allocate a buffer that is more than large enough to store the 16 digits of
  // precision requested below.
  char buffer[32];
  int len = -1;

// Print into the buffer. We need not request the alternative representation
// that always has a decimal point because JSON doesn't distingish the
// concepts of reals and integers.
#if defined(_MSC_VER) && defined(__STDC_SECURE_LIB__) // Use secure version with
                                                      // visual studio 2005 to
                                                      // avoid warning.
#if defined(WINCE)
  len = _snprintf(buffer, sizeof(buffer), "%.16g", value);
#else
  len = sprintf_s(buffer, sizeof(buffer), "%.16g", value);
#endif
#else
  if (isfinite(value)) {
    len = snprintf(buffer, sizeof(buffer), "%.16g", value);
  } else {
    // IEEE standard states that NaN values will not compare to themselves
    if (value != value) {
      len = snprintf(buffer, sizeof(buffer), "null");
    } else if (value < 0) {
      len = snprintf(buffer, sizeof(buffer), "-1e+9999");
    } else {
      len = snprintf(buffer, sizeof(buffer), "1e+9999");
    }
    // For those, we do not need to call fixNumLoc, but it is fast.
  }
#endif
  //assert(len >= 0);
  fixNumericLocale(buffer, buffer + len);
  return buffer;
}

// Changed sprintf_s format to 17
    std::string valueToString17(double value) {
  // Allocate a buffer that is more than large enough to store the 16 digits of
  // precision requested below.
  char buffer[32];
  int len = -1;

// Print into the buffer. We need not request the alternative representation
// that always has a decimal point because JSON doesn't distingish the
// concepts of reals and integers.
#if defined(_MSC_VER) && defined(__STDC_SECURE_LIB__) // Use secure version with
                                                      // visual studio 2005 to
                                                      // avoid warning.
#if defined(WINCE)
  len = _snprintf(buffer, sizeof(buffer), "%.17g", value);
#else
  len = sprintf_s(buffer, sizeof(buffer), "%.17g", value);
#endif
#else
  if (isfinite(value)) {
    len = snprintf(buffer, sizeof(buffer), "%.17g", value);
  } else {
    // IEEE standard states that NaN values will not compare to themselves
    if (value != value) {
      len = snprintf(buffer, sizeof(buffer), "null");
    } else if (value < 0) {
      len = snprintf(buffer, sizeof(buffer), "-1e+9999");
    } else {
      len = snprintf(buffer, sizeof(buffer), "1e+9999");
    }
    // For those, we do not need to call fixNumLoc, but it is fast.
  }
#endif
  //assert(len >= 0);
  fixNumericLocale(buffer, buffer + len);
  return buffer;
}

// relevant extract of decode function in jsoncpp
double decodeDouble(const std::string&  s)
{
    double value = 0;
    const int bufferSize = 32;
    int count;
    int length = s.length();

    // Avoid using a string constant for the format control string given to
    // sscanf, as this can cause hard to debug crashes on OS X. See here for more
    // info:
    //
    //     http://developer.apple.com/library/mac/#DOCUMENTATION/DeveloperTools/gcc-4.0.1/gcc/Incompatibilities.html
    char format[] = "%lf";

    count = sscanf( s.c_str(), format, &value );

    return value;

}

void double16()
{
    std::cout << "jsoncpp 0.60 implementation" << std::endl;

    double d;

    for(int i = 0; i < samplessize; i++)
    {
        d = samples[i];

        std::string s = valueToString16(d);

        double r = decodeDouble(s);     

        std::cout << "val: " << d << " dec: " << r << " dif: " << d - r << std::endl;
    }
}

void double17()
{
    std::cout << "fixed implementation" << std::endl;

    double d;

    for(int i = 0; i < samplessize; i++)
    {
        d = samples[i];
        std::string s = valueToString17(d);

        double r = decodeDouble(s);     

        std::cout << "val: " << d << " dec: " << r << " dif: " << d - r << std::endl;
    }

}


#if (STANDALONE > 0)
int main(int argc, char* argv[])
#else
int jsoncppBugReport(int argc, char* argv[])
#endif

{   
    std::cout << "decimal" << std::endl;

    std::cout   << std::scientific
                << std::fixed
                << std::setprecision(std::numeric_limits<double>::digits10 + 2);

    double16();
    double17();
#if defined(_MSC_VER)
    std::cout << "hex" << std::endl;

    std::cout   << std::hexfloat
                << std::setprecision(std::numeric_limits<double>::digits / 8 + 2);

    double16();
    double17();
#endif

    return 1;
}
@cdunn2001
Copy link
Contributor

I like this change. Submit as a pull-request, and let's see if anyone else offers a contradictory opinion.

But for the record, we never guarantee round-trip numerics, as jacobsa noted in #85.

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

Successfully merging a pull request may close this issue.

2 participants