Floating point rounding

[quicknir]

I'll preface this by saying I know there's been some discussion about this already, and by saying that my version is not the latest, so this may already be fixed. Right now, you can do:

auto j = json::parse(R"({"x":1.5})");
int x = j.at("x");

With no complaint. Is this intentional? I thought that json checks to see if precision is lost before converting to an integer.

[nlohmann]

The library behaves just like C++, meaning it interprets your code just like

auto x = 1.5;
int y = x;

We do store 1.5 as double internally, so we try to keep its precision. But if you decide to store it as an int, there is nothing JSON or the library will do about it.

[quicknir]

Hmm, if I compile with -Wfloat-conversion will a warning be raised? If you're busy I'll test this myself and get back to you; right now I'm using your library through our build system and it's a bit convoluted for me to see what the situation is with that flag.

[quicknir]

So I checked, and compiling with -Wfloat-conversion does not cause any warning, at least when json is included via -isystem, possibly it would be different with -I but I'm not sure. I don't really understand the rationale for this; these types of narrowing conversions happening quietly is rather widely regarded as a bad thing, that's why there's warnings and even why newer forms of initialization often disallow them. Obviously it's a breaking change but I think that version 3 should see this changed.

[nlohmann]

You can check for the type with is_number_float() and avoid conversions to integers. But whether the library should drop a warning would be something I like to discuss.

[nlohmann]

FYI: This is the currently used conversion code: https://github.com/nlohmann/json/blob/develop/src/json.hpp#L1084

[quicknir]

Another way to approach this problem, which you may be philosophically against, is to allow more arguments/control to the parsing of the json in the first place. Whether or not to throw when you try to extract negative to unsigned, or floating point to integer, could be controlled via optional arguments to parse/serialize functions, as well as other things like Nan/Infty parsing.

I think that with sane defaults this will not affect the usability of the library (I wouldn't really care whether implicit conversion or throwing is the default, Nan probably shouldn't be), but it will make it more easily usable for more people, and the C++ approach is generally to allow as much customization as possible.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[quicknir]

Hi, I'm disappointed that this has been closed without further discussion. I don't think that mimicking C++ behavior that itself is a more-or-less-widely-acknowledged Bad Thing (tm) that was inherited from C, is a good basis for a design decision. Silently throwing away user information is something that no good C++ library should do; it violates fail fast and makes debugging harder. There's two consistent positions here, IMHO:

1. Not allow conversions directly to integer at all. That means a statement like int x = j.at("x"); either doesn't compile, or (would depend on the C++ implementation details) would extract a double from the json that would then in C++ (as opposed to in library) implicitly convert to int. This way at least narrowing conversions would catch it. This is the most consistent in some sense with the spirit of json itself; json doesn't really do integers at all, it's number type is floating point, plain and simple. And you offload the problem into C++/user space, minimizing the number of choices the json library has to make.
2. Allow extraction as an integer, but throw if rounding is occurring. Note also that negative numbers have the same issue. Extracting a value of -1 into unsigned just silently wraps around. This is relatively easy to handle in the sense that the library is already making some intelligent decisions about how to store the parsed json value (since it will sometimes use an integral type, and sometimes a floating point type).

Either of these approaches I think is fine. Both of them at least are no less type safe than C++. Currently, this library is more type unsafe than C++ with typical warnings on. Those warnings were added as people realized all the issues with C playing fast and loose with typing. We can't change this for backwards compatibility reasons but new libraries should strive to be more type safe than C++, not less.

[nlohmann]

I reopened the issue, hoping that there are more people to discuss it.