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

add uint64 data to json-c #542

Merged
merged 4 commits into from Mar 1, 2020
Merged

add uint64 data to json-c #542

merged 4 commits into from Mar 1, 2020

Conversation

dota17
Copy link
Member

@dota17 dota17 commented Feb 25, 2020

Many people want to add uint64 data into json-c in #520, #326, #143. I believe it is necessary and useful, so I make it in this commit. Review please.

Added content:

  1. Add uint64_t c_uint64 data type and other related parameters, such as json_type_uint.
  2. Add new functions:
  • json_object_uint_to_json_string()
  • json_object_new_uint64
  • json_object_get_uint64()
  • json_object_set_uint64()
  • json_object_uint_inc()
  • json_parse_uint64()
  1. Modify functions: (add uint64_t data/type )
  • json_object_set_serializer(),
  • json_object_get_boolean()
  • json_object_get_int()
  • json_object_get_int64()
  • json_object_get_double()
  • json_object_equal()
  • json_c_shallow_copy_default()
  1. Modify json_tokener_parse_ex()function:

if the number is double, just parse it as double.
if the number is not double, then distinguish if it is negative.
----if it is negative, parse it as int64 data.
----if it is not negative , parse it as uint64 data. Last, judge numuint64 <= INT64_MAX,
--------if yes, transfer it as int64 data.

  1. Add related test cases.
    test_cast.c, test_compare.c , test_int_add.c, test_parse.c, test_parse_int64.c, test_set_value.c.

As you can see, I have modified many files and achived adding-uint64. Welcome your comments and suggestions

@hawicz
Copy link
Member

hawicz commented Feb 26, 2020

Your changes look decent, but defining a whole new json_type_uint seems like an unnecessarily large breaking change, just to get one additional bit of magnitude.
Yes, that's a fairly straightforward way of adding this support, but I'm thinking that it might be a bit better to keep just a single json_type_int type exposed, and distinguish the actual width of the number only internally within the library. e.g. int64_t c_int64; in the json_object structure would instead turn into something like:

union data {
    ...
    struct {
        union {
            int64_t c_int64;
            uint64_t c_uint64;
        } u_int;
        int is_unsigned;
    } c_int;
    ...
} o;

Which would allow things like json_object_get_type() to continue to return json_type_int, while also providing a easier way forward for future updates to expand the available magnitude beyond 64-bits (perhaps even arbitrary precision integers).

@dota17
Copy link
Member Author

dota17 commented Feb 26, 2020

ok, I know your meanings. It's really a good solution, I will try to change it.

@hawicz
Copy link
Member

hawicz commented Feb 26, 2020

Thanks. I expect this will have the side-effect of making all places where we currently refer to c_int64 a bit more involved, but I'm hoping that ends up not being too bad. If you're going to try this then I guess we'll see. :)

@dota17
Copy link
Member Author

dota17 commented Feb 26, 2020

All this is a digression, could you open the coveralls right for json-c, ha-ha. It is a interesting tool.

json_object.c Outdated Show resolved Hide resolved
json_object.c Outdated Show resolved Hide resolved
json_object.c Outdated Show resolved Hide resolved
json_object.c Outdated Show resolved Hide resolved
json_object.c Outdated Show resolved Hide resolved
json_object.c Outdated Show resolved Hide resolved
json_object.c Outdated Show resolved Hide resolved
json_object.h Outdated Show resolved Hide resolved
@dota17 dota17 force-pushed the adduint64_final branch 2 times, most recently from b166b06 to af3e38e Compare February 27, 2020 12:44
json_object.c Outdated Show resolved Hide resolved
json_object.c Outdated Show resolved Hide resolved
json_object.c Show resolved Hide resolved
json_object.c Outdated Show resolved Hide resolved
json_object.c Show resolved Hide resolved
@dota17 dota17 force-pushed the adduint64_final branch 2 times, most recently from 03040a9 to c192df7 Compare February 28, 2020 09:37
@hawicz
Copy link
Member

hawicz commented Mar 1, 2020

I think this could still use a couple more tweaks (i.e. improved assert calls, a bit of simplification in object_equal), but it's good enough to merge. I'll make those other changes later today.

@hawicz hawicz merged commit 737aee4 into json-c:master Mar 1, 2020
@dota17
Copy link
Member Author

dota17 commented Mar 2, 2020

ok, thx.

@hawicz
Copy link
Member

hawicz commented Mar 2, 2020

Done (commit b3296e7)

@dota17
Copy link
Member Author

dota17 commented Mar 2, 2020

good,It's improved a lot. And #520 , #326 , #143 maybe should be closed.

@dota17 dota17 deleted the adduint64_final branch April 17, 2020 02:28
ploxiln added a commit to ploxiln/json-c that referenced this pull request May 10, 2020
ploxiln added a commit to ploxiln/json-c that referenced this pull request May 10, 2020
@andrewhodel
Copy link

Can't you just input an int64 then cast it as unsigned when you read it?

@hawicz
Copy link
Member

hawicz commented Mar 23, 2021

Input where, in your json input? That means you'd have things like:

{ "foo": -9223372036854775808,
   "bar":  -1 }

when you really mean:

{ "foo": 9223372036854775808,
   "bar": 18446744073709551615 }

Which would be quite confusing, not to mention not at all interoperable with any other json implementation that might support numbers that large.

@andrewhodel
Copy link

andrewhodel commented Mar 23, 2021 via email

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 this pull request may close these issues.

None yet

3 participants