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

is_number_unsigned() returns false for positive integers (int or 0 or 1 literals) #1885

Closed
ibc opened this issue Dec 29, 2019 · 10 comments
Closed

Comments

@ibc
Copy link

ibc commented Dec 29, 2019

  • What is the issue you have?

JSON fields whose value are assigned as a int or intN_t or literals without u (such as 0 or 1) make is_number_unsigned() to return false.

  • Please describe the steps to reproduce the issue. Can you provide a small but working code example?

I create a C++ JSON object and fill a field with 0 or 1 as follows:

json["foo"] = 0;
// or
json["foo"] = 1;

Then I check foo as follows:

auto ret = json["foo"].is_number_unsigned();
  • What is the expected behavior?

I want to assume that ret is true since json.foo is 0 or 1.

  • And what is the actual behavior instead?

ret is false, probably because json.foo holds a "signed" number (even if it's zero or a positive number).

  • Which compiler and operating system are you using? Is it a [supported compiler]
Apple clang version 11.0.0 (clang-1100.0.33.16)
Target: x86_64-apple-darwin18.7.0
  • Did you use a released version of the library or the version from the develop branch?

The latest 2.11.1. It happens with previous versions too.

More

I really expected that is_unsigned_number() would check the value of the element and not just the value type.

@ibc ibc added the kind: bug label Dec 29, 2019
@nlohmann
Copy link
Owner

nlohmann commented Dec 29, 2019

Indeed is_unsigned only checks the type and not the value. That is, if you pass a signed value (like the literal 0), the value is stored as number_integer_t (which defaults to std::int64_t). We do not perform overhead to cast positive signed values to unsigned values.

I am afraid the behavior will stay as is, because changing it would break existing implementations. Any proposal to fix the documentation?

@ibc
Copy link
Author

ibc commented Dec 30, 2019

That is, if you pass a signed value (like the literal 0), the value is stored as `number_unsigned_t``

Is that a typo? Did you mean number_t?

@ibc
Copy link
Author

ibc commented Dec 30, 2019

IMHO the docs may say exactly that:

is_number_unsigned() just returns true when the value was filled with an unsigned integer type (uint8_t, unsigned in, etc). If a literal (0 or 1) or a signed type (int8_t, int, etc) is given, then is_number_unsigned() will return false.

BTW what I wonder now is how is_number_unsigned() is useful at all for the application. If the json lib does not check the real value then this must be done by the app, and hence I don't see any value in having this checker which is confusing and unreliable.

ibc added a commit to versatica/mediasoup that referenced this issue Dec 30, 2019
- Do not rely on is_number_unsigned() of json C++ lib, which is
  unreliable due to its design. See:
    nlohmann/json#1885
@nlohmann
Copy link
Owner

nlohmann commented Jan 3, 2020

Is that a typo? Did you mean number_t?

Yes. Sorry. I edited the comment to avoid further confusion.

@nlohmann
Copy link
Owner

nlohmann commented Jan 3, 2020

The distinction into signed and unsigned numbers is done to accept a broader range of integer types during parsing. The check function was introduced in v2.0.0 when unsigned numbers were introduced.

The check functions can then be used to postprocess the parsed values. I do not share your assessment that it is unreliable.

Please let me know if there is anything left to do in this issue.

@ibc
Copy link
Author

ibc commented Jan 8, 2020

The problem I see is the following:

auto json = R"(
{
  "foo": 0,
  "bar": 1,
})"_json;
  • Does json["foo"].is_number_unsigned() return true or false?
  • Does json["bar"].is_number_unsigned() return true or false?

The answer is true in both cases.

If later I do:

unsigned int foo2 = 0;
unsigned int bar2 = 1;

json["foo2"] = foo2;
json["bar2"] = bar2;

then, both foo2 and bar2 are also number_unsigned.

However, if I do:

json["foo3"] = 0;
json["bar2"] = 1;

then, both foo3 and bar3 are NOT number_unsigned.

This is, depending on how values are inserted into the JSON object, we get different results when checking those values once in the JSON.

I understand that this json lib stores the "native" type into each JSON value. I just wonder why. Once values are inserted into a JSON object, which the native type was should not be relevant at all IMHO. So an API that exposes which the native type was (such as is_number_unsigned()) does not make much sense to me.

@nlohmann
Copy link
Owner

nlohmann commented Jan 8, 2020

This is, depending on how values are inserted into the JSON object, we get different results when checking those values once in the JSON.

Exactly. The function does not take the value, but the stored type into account.

I understand that this json lib stores the "native" type into each JSON value. I just wonder why.

The reason is that the library should be able to store both negative and very large integer values without falling back to floating-point numbers. Thereby, we have the range -9223372036854775808 to 18446744073709551615 rather than "just" the range of std::int64_t.

Once values are inserted into a JSON object, which the native type was should not be relevant at all IMHO. So an API that exposes which the native type was (such as is_number_unsigned()) does not make much sense to me.

If you store the value manually (i.e., not during parsing), then you have the control of which type is used - just as in your example:

json j_signed = int(1);
json j_unsigned = unsigned(1);

@ibc
Copy link
Author

ibc commented Jan 9, 2020

Thanks. The problem I see is: how to know which type will be assigned to a number during parsing? i.e.:

auto json = R"(
{
  "foo": 0,
  "bar": 1,
  "baz": -1
})"_json;
  • Will json["foo"] be always number_unsigned?
  • Will json["bar"] be always number_unsigned?
  • Will json["baz"] be always number_integer?

@nlohmann
Copy link
Owner

nlohmann commented Jan 9, 2020

how to know which type will be assigned to a number during parsing?

Well, essentially by calling is_number_unsigned() or checking the type() enum.

The parser will use number_unsigned_t for all positive integers (including 0) if they fit into 64 bits. It will use number_integer_t for negative integers if they fit into 64 bits. In both cases, it falls back to double otherwise.

This, however, should be an implementation detail. You can always case to your preferred type.

@ibc
Copy link
Author

ibc commented Jan 9, 2020

Clear, thanks. Let's close this.

@ibc ibc closed this as completed Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants