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 a string version of numbers #580

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

schmidtw
Copy link

This change adds storing the string version of numbers during decoding
so that large or precise numbers can be handled by the program without
the need for cJSON to directly deal with the numbers. There is still a
63 character limit, but this change enables applications to handle 64
bit numbers while enabling cJSON to maintain C89 support.

This change adds storing the string version of numbers during decoding
so that large or precise numbers can be handled by the program without
the need for cJSON to directly deal with the numbers.  There is still a
63 character limit, but this change enables applications to handle 64
bit numbers while enabling cJSON to maintain C89 support.
@schmidtw
Copy link
Author

schmidtw commented Sep 6, 2021

@Alanscut any thoughts on adding this feature? It allows cJSON to punt on large number handling to the users. The problem of 64bit numbers not being supported keeps growing worse as more users move to larger numbers because they are widely supported today.

For some projects I'm working on I have been waiting for feedback or acceptance of this PR, but if the project isn't adding new features then I'll find alternatives.

@VelGer
Copy link

VelGer commented Sep 8, 2021

We would also benefit from this feature.

@Alanscut
Copy link
Collaborator

@Alanscut any thoughts on adding this feature? It allows cJSON to punt on large number handling to the users. The problem of 64bit numbers not being supported keeps growing worse as more users move to larger numbers because they are widely supported today.

For some projects I'm working on I have been waiting for feedback or acceptance of this PR, but if the project isn't adding new features then I'll find alternatives.

@schmidtw Thanks your great contribution! I'm sorry for the late reply. Limited by the support for c89, it is not perfect when processing numbers. I once thought about whether it is necessary to abandon the support for c89, but considering that this breaking change will have an adverse effect on many existing embedded software, so I give up.

I think your approach is a good way, while taking into account the support of 64-bit numbers and c89. one little thing, not everyone hope to store the number as valuestring, should we add a new function to implement this?

@VelGer
Copy link

VelGer commented Oct 22, 2021

@Alanscut Did you mean:

  1. To have a function (like "cJSON_GetNumberValueAsString()") that delivers the Number value as string? Or
  2. To have a function with which the functionality (making number available as string in the structure during parsing) can be turned on?

I think implementing both can be a good idea, so that in the normal case, the functionality is not active and the number is not stored as string (this maintins backwards compatability and reduces CPU load), but if the user wants to be able to read the number value as string, he can turn the functionality on, then parse his input JSON String and finally read the number as string using a method like "cJSON_GetNumberValueAsString()".

Is there a struct in memory where such cJSON configuration details are stored? What do you guys think?

@schmidtw
Copy link
Author

That's good feedback. Looking at the patch again, it is making a strdup() call for each integer even if it perfectly converts. I'm going to do some work on this (performance) & see if I can add a function or two to ease useage.

…and supports arbitrary length number strings.
@schmidtw
Copy link
Author

schmidtw commented Oct 26, 2021

It's a bit more code than I had hoped, but this provides 4 new functions and keeps the existing behavior otherwise.

cJSON_ParseWithLengthNumStringOpts();
cJSON_GetNumberValueAsString();
cJSON_CreateNumberAsString();
cJSON_SetNumberValueAsString();

These will validate the the input string is a legit JSON number before it lets you set it. They also will update the double and int versions. If you want number strings you get them all or you (default) don't get any. Trying to figure out if a number is exactly represented is fairly hard given the way the system is assembled, so it's all or none.

I added some changes locally to verify I have tests to cover the new code. Happy to share - it just seemed like a separate PR.

Thoughts?

@schmidtw
Copy link
Author

schmidtw commented Nov 2, 2021

@VelGer or @Alanscut any thoughts?

@VelGer
Copy link

VelGer commented Nov 19, 2021

@schmidtw Sorry for the delayed response, I have been out of the office for a while. I think it looks good, it keeps backwards compatability and works like I would expect.

What do you think about making the parsing function expandable by giving it a general name and replacing all the optional bools with a single int. So that functionality can be turned on / off by using bitwise operators? This way we can have a single parsing function that can be used with future functionality expansions:

We can then add a define for each function that the user can enable, like:

#define CJSON_REQUIRE_NULL_TERMINATED (1 << 0)
#define CJSON_ENABLE_NUMBER_STRINGS (1 << 1)

Usage would then be:
cjsonStruct = cJSON_ParseOpts("{ ...JSON String... }", CJSON_REQUIRE_NULL_TERMINATED & CJSON_ENABLE_NUMBER_STRINGS );
or:
cjsonStruct = cJSON_ParseOptsLen("{ ...JSON String... }", 45, CJSON_REQUIRE_NULL_TERMINATED & CJSON_ENABLE_NUMBER_STRINGS );

@schmidtw
Copy link
Author

@VelGer I don't have any strong opinions on the cJSON_ParseOpts() ... but I'd like to get the functionality generally in. I've been busy with some other things, but if you want to send my repo a PR I'd be happy to review/merge.

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.

3 participants