locale issues still not fixed

[jeroenwalter]

Hi

We're using jsoncpp 1.6.2. on windows 64bit in visual studio 2013.
We (and, I think, many others) use setlocale to set the locale of the program to that of the current user (in Windows).

According to some closed issues on this board, locale issues should be fixed now.
For instance, #89 and #3 to me imply locale issues should be fixed.
However, they aren't.
There is a horrible workaround in the valueToString method that replaces all commas with a period....really ??? That's not a solution, that's a hack.
Furthermore, all decodeDouble methods still use a sscanf call that depends on the current runtime locale.

For instance, this code does not parse the value 1.234 correctly:

setlocale(LC_ALL, "fr-FR"); // change to a locale that uses comma instead of period.
std::string content = "{ \"bla\" : 1.234 }";
std::stringstream s(content);

Json::CharReaderBuilder rbuilder;
rbuilder["collectComments"] = false;
std::string errs;
Json::Value root;

bool ok = Json::parseFromStream(rbuilder, s, &root, &errs);
double value = root["bla"].asDouble();

value will be 1.000000

A correct solution would be using a stringstream solution proposed before, or to use the locale specific variants of sprintf and sscanf.
In VS2013 these are _sprintf_l and _sscanf_l. I don't know if these are available for gcc, maybe you can find out.
Feed those methods a locale specifically created for jsoncpp, for instance with the following method:

_locale_t getJsonLocale()
{
static _locale_t json_locale = _create_locale(LC_ALL, "C");
return json_locale;
}

I would appreciate it if you can take the time to fix this problem once and for all without resorting to assume that everyone uses the same "C"/english locale and without doing string replacements after a printf.....

[cdunn2001]

I'm only a referee on this. We've had problems with older VisualStudio versions, and with compiler standards compliance in general.

That said, I wish I could get people to switch to the new builders. No-one should use valueToString() directly. It necessarily depends on global state. It is unfortunately still used within Value::asString() for numbers, but in theory a StreamWriter could be configured with locale information. The CharReader too. Configuration is absolutely required for locale support because, technically, it's not part of the JSON standard. The standard clearly prescribes periods for decimal separation. So auto-detecting the locale is actually incorrect. But I favor flexibility. That's the whole purpose of the new Builders.

How to get the changes to compile everywhere - all platforms, all compilers, all fairly recent versions -- is another story. But if you submit something, we could at least try it, or argue it out with anyone who breaks. If we restrict the changes to the 1.y.z (i.e. master) branch, then we do not have to support older compilers.

[jeroenwalter]

As you said, the json standard prescribes periods for decimal separation, so why should the locale even be an issue in jsoncpp and its provided readers and writers?
Maybe I'm missing something, but what you're basically saying is that jsoncpp has no default parser/generator that is locale-independent and requires configuration of some poorly documented concept of StreamWriters before becoming useful.
If there is any useful documentation including examples, please point me to it, I couldn't find any and the source code doesn't help either.

Like I suggested, the provided readers and writers can be fixed with the locale specific functions of sprintf and sscanf, that's what we did (we were upgrading from 0.6) but I'm not sure if those methods are available with gcc.

Btw, I don't have a large issue with StreamWriter, but I do have one with the Reader and CharReader as they use scanf.

[cdunn2001]

some poorly documented concept of StreamWriters

Please update the wiki or submit documentation changes. I like the Builder concept, but I don't know how to explain it to people. We cannot simply remove the old code. The Builders are vital for adding new features without breaking backward-compatibility.

I agree that scanf is bad, but the most difficult part of this project is maintaining compatibility for multiple users. I test on my Mac. We now have both Windows and Linux CI testing. But with so many platforms/compilers/versions, there is only so much we can do. If you have specific ideas, submit a patch. As long as nobody breaks, I'm sure we're all fine with your suggestions and we would welcome the help.

[jeroenwalter]

I really don't see the point in the StreamWriter and the CharReader classes/interfaces, they are nearly empty interfaces.
As I see it, you want people to create their own StreamWriters and CharReader subclasses. This would require them to do all the JSON parsing/generation themselves, which defeats the purpose of a JSON library IMO.
Your current implementation of the CharReader class uses an exact copy of the deprecated Reader class, OurReader. So why is this CharReader class even present?
The StreamWriter and CharReader interfaces don't offer any enchanced functionality over the deprecated classes.
I'm missing the problem that the builders are trying to solve. So if you don't know how to explain it to people, answer that question first, then give a concrete example of a new StreamWriter subclass and CharReader subclass that actually have some new functionality that is not already present in the deprecated reader/writer.

As for backwards compatibility, you already require C++11 in jsoncpp 1.6.2. Old compilers won't work with that, so I guess backwards compatibility is only an issue with jsoncpp 0.6 ?

I'll try to create a patch for the changes I did, but I'm not very familiar with git, so I don't know if that will be a success.

[cdunn2001]

Your current implementation of the CharReader class uses an exact copy of the deprecated Reader class, OurReader. So why is this CharReader class even present?

For future changes. We cannot modify the old Reader because it would break binary-compatibility.

• OurReader is completely internal, so we can change it dramatically if we want.
• OurCharReader accepts the collectComments feature only at configuration time, which decouples parse() from that feature.
• See also BuiltStyledStreamWriter.

I'm missing the problem that the builders are trying to solve.

It has been very difficult to fix bugs and add features to this library without breaking binary-compatibility.

As I see it, you want people to create their own StreamWriters and CharReader subclasses.

No! Maybe that's not clear, but no-one ever needs to derive their own implementations. Then can, but likely do not need to.

I'm missing the problem that the builders are trying to solve.

Maintainability. I learned the value of the Builder Pattern when I worked at Amazon. It decouples configuration from function -- without resorting to telescoping constructors. It's much less common in the C++ world, and that's a shame.

As for backwards compatibility, you already require C++11 in jsoncpp 1.6.2. Old compilers won't work with that, so I guess backwards compatibility is only an issue with jsoncpp 0.6 ?

0.6 works with lots of old compilers, yes. But we try to maintain backward-compatibility of 1.6 for all its current users as well.

I'm not very familiar with git.

It's worth learning!

[jeroenwalter]

So, in order to achieve binary compatibiliy, you created the Builder and the near empty StreamWriter and CharReader interfaces. Not with the intention for people to inheret from them, but to allow people to configure the default implementations.
I'd say that using the Builder pattern with a separate class here adds a layer of complexity that is not needed and only confuses things.
I would've integrated the configuration functionality provided by the Builder directly in StreamWriter and CharReader.
But that's just my preference.

In the mean time, I've fixed the locale dependency. It works for Windows, should work for gcc also.
It uses stringstream instead of printf and scanf.
See this commit jeroenwalter@ae26094

I created a pull request, but that failed.
https://travis-ci.org/open-source-parsers/jsoncpp/jobs/64777276

I think it failed because some tests don't pass. This is caused by the python testrunner, that uses string compares for float values. It should not do that.
A value of 12.3 is output via stringstream as 12.300000000000001
printf may output 12.3 but I think you've just been lucky all this time that a string compare went ok.
The same goes for the input of a double with scanf and stringstream.
Another reason why a string compare is incorrect is already shown in the method normalizeFloatingPointStr in the jsontestrunner. This is a workaround for the different implementations of outputting a double to text. This already should've been a sign on the wall that double output/input and comparison are tricky.

[cdunn2001]

I would've integrated the configuration functionality provided by the Builder directly in StreamWriter and CharReader.

Technically, adding a virtual method breaks binary compatibility. You're at the mercy of compiler implementation.

Definitely, adding a data member breaks it.

You could avoid these problems by wrapping the implementation (pimpl idiom or just a pointer to base-class) a common solution in C++ traditionally. But you still have the problem of "telescoping constructors". And there are other advantages. Our maintenance burden is now much reduced. When the defaults are fine, anyone can use operator<<(ostream, Value) and operator>>(istream), so I'm not worried about issues of convenience.

python tests

I don't know the purpose of the Python tests. I concentrate on the unit-tests. But maybe there are some useful functional tests of standard JSON inputs. You're looking at some legacy code. If you have time to clean it up, I encourage that. I hope you have a way to check coverage too, since I'd rather not lose any tests for otherwise uncovered code. Also, if you could put some thought into whether your changes can be merged to the 0.y.z branch without breaking binary-compatibility, that could be very helpful too. But really, I would rather see improvements to the Python tests first, as a separate pull-request, along with coverage reports.

locale changes

See discussions at #3, #7, and #9. You'll notice that someone tried almost exactly your changes a year ago, but I had to revert because of WinCE compatibility. The discussion we need is whether we need all that compatibility on the 1.y.z branch. The downside is that 1.y.z and 0.y.z would diverge. In the past, I worried about the difficultly of applying a fix to both branches, but we don't seem to get significant bug-reports anymore. So maybe it is time to let 1.y.z diverge completely. I'm not sure.

[jeroenwalter]

I'm confused here. You are the owner of this project, how can you not know what the Python tests are for?
They, together with the jsontestrunner, are there for checking if jsoncpp parses and generates JSON correctly using a predefined set of json files (http://json.org/JSON_checker/). The error it makes is that is uses string comparisons to check the output with an expected output, which may fail for double/float values. After my changes, they do fail, though in my opinion (I could be wrong here) the generated JSON is still valid.
I don't know Python, so I can't help you there.

Regarding the WinCE compatibility in 1.y.z, if WinCE can't handle C++11, drop support for WinCE.
Maybe you should create a 2.y.z version and stop development on 0.y.z and 1.y.z, so you can get rid of all deprecated stuff.
It's no use of marking classes as deprecated if you don't actually plan or are able to remove them ever.

I won't merge the changes back into 0.y.z, because if 0.y.z should run on even more older compilers then I have no way of testing if the changes won't break something.

[cdunn2001]

Please excuse any incoherence. I am very tired.

I'm confused here.

Whenever I fix a bug, I write a unit-test. There is overlap between the unit-tests and the Python tests, but I don't know how much.

The error [the python test-suite] makes is that it uses string comparisons to check the output with an expected output, which may fail for double/float values. After my changes, they do fail, though in my opinion (I could be wrong here) the generated JSON is still valid.

In the case of floating-point numbers, it's not clear what's "correct". JSON describes how floats are represented, but it says nothing about equivalence. Technically, the standard does not limit the lengths of floats or integers, so the IEEE conversions are just a particular implementation. Ensuring stability of output is more important than cleaning up the code.

[From earlier comment.] I think you've just been lucky all this time that a string compare went ok.

Not luck. A lot of effort by several contributors recently went toward ensuring that floats are identical after a round-trip. The trick is to use 17 digits in printf, iirc. That's harder to guarantee with stringstream. Actually, a co-worker recently ran into a nasty problem with setting precision for ostream.

The JSON tests, as you've seen, are brittle. They need help.

It's no use of marking classes as deprecated if you don't actually plan or are able to remove them ever.

We definitely plan to remove them in 2.y.z someday.

I won't merge the changes back into 0.y.z, because if 0.y.z should run on even more older compilers then I have no way of testing if the changes won't break something.

Exactly. This might be a good moment to let 1.y.z and 0.y.z diverge. I think we should include as much of our backlog as possible in a new 2.y.z branch, maybe including the stringstream change. But it would take a few weeks, at least.

But let's consider the problem you are trying to solve. The comma-hacks seems to be working, so that's not it. For scanf, the caller can set the locale. For the error with 1.234, simply set "strict mode", via the Builder; you will at least catch the failure to read the entire value. In other words, there is no immediate bug. Just annoyances. And fixing the annoyances would change behavior, right? So these changes belong in 2.y.z. I'd like to see a test-coverage report before I even think about a major-version bump. But eventually yes, stringstream is probably the way to go.

When we are ready for 2.0, we'll include your changes. You can simply leave this open.

[David-Defoort]

This is an old issue but it is still a problem
On Windows, with those regional settings:

You can end up with a quote as separator (yes this is rare and edge case, but some people may end up with those settings 🤷 ).
Then the hack to replace comma with period does not do anything and you end up with invalid json:

"DoubleValue" : 0'35475578904151917.0

See how it contains a quote and a trailing .0 😕

This is due to those hacks:

jsoncpp/src/lib_json/json_writer.cpp

Line 155 in 94cda30

buffer.erase(fixNumericLocale(buffer.begin(), buffer.end()), buffer.end());

And then:

jsoncpp/src/lib_json/json_tool.h

Lines 89 to 101 in 94cda30

	/** 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
	*/
	template <typename Iter> Iter fixNumericLocale(Iter begin, Iter end) {
	for (; begin != end; ++begin) {
	if (*begin == ',') {
	*begin = '.';
	}
	}
	return begin;
	}

[jeroenwalter]

We gave up on this json library and switched to JSON for Modern C++: https://github.com/nlohmann/json
Maybe this may help you too.