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

Realloc improvement in par_shapes #44

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

Conversation

SasLuca
Copy link

@SasLuca SasLuca commented Feb 28, 2020

Rationale

Due to the design of PAR_REALLOC it can't work with some kinds of allocators that are not designed to support realloc. A good way to solve this, as seen in other libraries such as stb, is to have the PAR_REALLOC macro also provide the old size of the buffer. I specifically had this issue with par_shapes so I only provided a solution for that header.

Changes

I changed the PAR_REALLOC macro to accept an extra parameter OLD_SZ that represents the old size.
The default implementation of the macro remains unchanged.
I then searched for all the uses of PAR_REALLOC in par_shapes and added the old size of the buffers to the PAR_REALLOC call. This sometimes involved creating new variables such as int old_dst_npoints = dst->npoints;.

Testing

I tested my solution by adding a small realloc wrapper in test_shapes.c which uses malloc and free and overriding the PAR_* allocation macros.

All tests have passed when I ran them after I applied my changes.

Considerations

This will affect people who already override PAR_REALLOC since they will have to add the extra parameter to the macro, however, if their PAR_REALLOC implementation already works without needing the old size then they can just ignore the OLD_SZ parameter, this is what the default implementation for PAR_REALLOC does too.

This will cause an inconsistency in the API however since all other par headers will still have the old-style realloc. If needed I can provide this change for the other par headers as well.

@SasLuca
Copy link
Author

SasLuca commented Feb 28, 2020

Not sure why the CI build fails, it seems like it's not testing with my changes.

@prideout
Copy link
Owner

prideout commented Mar 1, 2020

Nice, this change makes sense. This is the PAR_MALLOC portion of the par headers, which is duplicated across many of the older par headers, so it fails CI due to another par header's definition.

To fix the CI you can either update all the other par libraries that have a PAR_MALLOC block, or you can leave PAR_RESIZE as-is and put this functionality into a macro with a new name. If you decide to make a new macro, make sure it is outside the PAR_MALLOC block.

Btw, I recommend par_octasphere over par_shapes if you are concerned about memory allocations.

@SasLuca
Copy link
Author

SasLuca commented Mar 2, 2020

Thanks for the feedback.
I will look into adding this to the other par headers in the future, but at the moment I might just add a new macro in par_shapes and resubmit the request.

This change was motivated because I am working on a library that provides full control over memory allocations using allocators and I use par_shapes as a dependency.

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.

2 participants