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

Custom allocators with context. #63

Closed
Matthew-Jemielity opened this issue Oct 25, 2016 · 3 comments
Closed

Custom allocators with context. #63

Matthew-Jemielity opened this issue Oct 25, 2016 · 3 comments

Comments

@Matthew-Jemielity
Copy link

Hi,

Use of some custom allocators requires passing a context object with (mutable) allocator state. Currently parson doesn't really support such allocators and trying to get a saved state would require for example playing with static variables.

I forked the project and made an implementation that allows that, which you can check at my repo: https://github.com/Matthew-Jemielity/parson . It doesn't change the existing API, but adds some new types and a function. I'm creating this issue as per readme to discuss it. If you think the change is useful, I'll make a pull request.

Regards,
Matt.

@kgabis
Copy link
Owner

kgabis commented Oct 27, 2016

In this implementation you're still using a static variable but it's hidden behind parson's API. How is that better than just referencing some static context from custom malloc/free functions? e.g.

static custom_alloc_ctx_t ctx;

void* custom_malloc_wrapper(size_t s)
{
    return custom_malloc(&ctx, s);
}

void custom_free_wrapper(void *p)
{
    custom_free(&ctx, p);
}

int main()
{
    json_set_allocation_functions(custom_malloc_wrapper, custom_free_wrapper);
}

I might be missing something but I don't think adding json_set_allocation_ctx_functions to the API is necessary because you could achieve same functionality with existing API.

@Matthew-Jemielity
Copy link
Author

Matthew-Jemielity commented Oct 27, 2016

My thinking was that the users may not want or be able to use static variables in their code to hold the context.
You're right that it's hiding static variables in parson. Currently parson holds the allocator function pointers as static variables and my implementation adds pointer to context as I was aiming for smallest changes required for this to work. To really fix that parson would need to have some state object returned to user and passed to virtually all functions and remove the use of static variables altogether. Something like:

typedef struct {
    JSON_Allocator_Ctx * ctx;
    JSON_Malloc_Ctx_Function alloc_;
    JSON_Free_Ctx_Function free_;
} JSON_Parson_State;

int main(void)
{
    JSON_Parson_State state = json_init_parson_state();
    json_set_allocation_functions(&state, custom_malloc_wrapper, custom_free_wrapper);

    JSON_Value * v = json_parse_file(&state, "file.json");
    /* ... */
    json_value_free(&state, v);
    /* and so on */
}

So this would mean far greater changes to the API. If you agree to such changes, I could prepare a patch.

@kgabis
Copy link
Owner

kgabis commented Oct 27, 2016

I realize that the only way would be to add some context to every function call but I strongly object against such change. I'm going to close this issue because the original functionality you suggested can be easily achieved using current API and "users may not want or be able to use static variables" is not very convincing.

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

2 participants