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

Need public API to free returned strings (e.g. cJSON_Print) #5

Closed
ewmailing opened this issue May 11, 2016 · 13 comments
Closed

Need public API to free returned strings (e.g. cJSON_Print) #5

ewmailing opened this issue May 11, 2016 · 13 comments

Comments

@ewmailing
Copy link

There should be a public API to free returned strings like from cJSON_Print().

My usage case is that I'm writing a middleware library that uses cJSON. The user is free to use cJSON elsewhere in their project. I don't necessarily know what memory allocator they are using, so when I call cJSON_Print(), I don't know what free() function to call. cJSON_free is private/internal.

I'm thinking either adding a cJSON_FreeString() which wraps cJSON_free, or making CJSON_free public are the easiest solutions.

Thank you

@DaveGamble
Copy link
Owner

You should implement cJSON_Hooks and discourage them from doing the same.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Nov 12, 2016

I propose adding cJSON_Hooks as optional parameter to all the functions that are doing frees/allocations. That would be part of the breaking changes for 1.0.0

@FSMaxB FSMaxB reopened this Nov 12, 2016
@FSMaxB FSMaxB added this to the 1.0.0 milestone Nov 12, 2016
@FSMaxB
Copy link
Collaborator

FSMaxB commented Nov 12, 2016

What is passed will be a pointer to cJSON_Hooks and if null, regular malloc and free will be used.

@DaveGamble
Copy link
Owner

Nice.

@DaveGamble
Copy link
Owner

While you're there, it might be wise to remove unbuffered printing entirely, especially since you have ensure() working nicely now :)

@ajaybhargav
Copy link
Contributor

I would suggest rename cJSON_Delete to cJSON_DeleteObject and create a new API cJSON_DeleteBuffer which frees the print buffer using hook. Let me know I can create patch for that.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Nov 17, 2016

No. By allowing the user to pass in a pointer to an instance of cJSON_Hooks, the user of the library knows which allocator was used for allocating the printbuffer, so no special function is required for that, just use the deallocator that fits the allocator you gave to cJSON for printing.

@mjerris
Copy link
Contributor

mjerris commented Feb 27, 2017

Is there currently any way using cJSON_Hooks to pass along another void pointer that could go to the allocator? I can't see a way. This would be necessary to do something like using a memory pool allocator? Obviously this would require major upheaval to the api, but seems if this is going to happen, now would be the time. Is there any interest in looking at that now, or would it be too invasive to make sense?

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 27, 2017

This sounds feasible for v2. My idea was to have an extended cJSON_Hooks struct that get's passed in (as pointer) to every function that allocates memory. If left NULL, defaults would be used. That new cJSON_Hooks is also supposed to have a pointer to a realloc function (optional) and may also contain the error pointer (or rather error position).

Allocator functions that take a void pointer could be added as separate fields to this struct (or be the default, not sure).

I will definitely put this feature on the list.

@mjerris
Copy link
Contributor

mjerris commented Feb 27, 2017

Yes.. this approach makes sense... the other way to do it.. which probably has value, is to include a concept of a "parent" object, and work the api to pull that void pointer from the parent object. Having access to the parent to be able to walk back up to the root json may also have other unexpected advantages. Anyway all food for thought. We are not using anything like this now, but have at least talked about how it would be useful.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 27, 2017

Interesting idea. But I don't think this works for allocators because any object can be detached from the tree and become it's own root at any time.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Apr 12, 2017

I added cJSON_malloc and cJSON_free on the develop branch. Since version 2 is still way ahead I decided to just add simple wrappers that call the internal functions. They will be removed again in version 2.

This is still quite prone to race conditions though.

@FSMaxB FSMaxB removed this from the 2.0.0 milestone Apr 30, 2017
@FSMaxB
Copy link
Collaborator

FSMaxB commented May 2, 2017

cJSON_malloc and cJSON_free are now available with version 1.5.0

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

No branches or pull requests

5 participants