Fixes #1: added custom allocator support and removed exit(1) on allocation error - #2
Fixes #1: added custom allocator support and removed exit(1) on allocation error#2RaphGL wants to merge 7 commits into
exit(1) on allocation error#2Conversation
zfletch
left a comment
zfletch
left a comment
There was a problem hiding this comment.
Thanks for making this PR! The underlying structure of the code looks good and I think we should be able to merge this with some minor modifications. I do have a few questions and some change requests.
| static size_t znext_size_index(size_t size_index); | ||
| static size_t zprevious_size_index(size_t size_index); | ||
| static struct ZHashTable *zcreate_hash_table_with_size(size_t size_index); | ||
| static void *zmalloc(size_t size); |
There was a problem hiding this comment.
What's the reasoning for removing zmalloc instead of rewriting it to work with the allocator? I think we should treat zmalloc, zfree and zcalloc the same: either they should all be static functions or they should all be inline.
There was a problem hiding this comment.
You had a zmalloc so that you could just ignore the return value. Since the return is propagated the zmalloc doesn't do anything it would just end up being this:
void *zmalloc(struct ZAllocator allocator, size_t size) {
return allocator.alloc(size);
}Or it would just return malloc(size). I removed all instances of allocator use to be based on the alloc and free functions. This means all your allocator has to support is alloc and free. You could technically also just give a no op function as free and then externally call myalloc_free_all(alloc) or something.
So I wrapped zcalloc so that we could still do zero initialized memory allocation without relying on any external API.
There was a problem hiding this comment.
The issue is that zcalloc exists but zmalloc and zfree have been removed, which still works from a technical perspective, but is inconsistent.
How about adding zalloc and zfree as static functions that take struct ZAllocator allocator, like zcalloc?
There was a problem hiding this comment.
I think adding zcalloc, zmalloc and zfree wouldn't do anything for code clarity here and they would just be wrapping the equivalent functions in struct ZAllocator with no added benefit.
zcalloc was kept only because I did not want to write memset every time I called allocator.alloc. Personally I don't see a problem with it, but if you want to I could change it to a clearer name like zalloc_zeroed or something just to be clearer that it's just a helper function or I could remove the helper function altogether and call `memset every time I need it (zcalloc is only called twice in this codebase).
Either way, let me know what you sounds better to you and I'll make the changes.
| static size_t znext_size_index(size_t size_index); | ||
| static size_t zprevious_size_index(size_t size_index); | ||
| static struct ZHashTable *zcreate_hash_table_with_size(size_t size_index); | ||
| static void *zmalloc(size_t size); |
There was a problem hiding this comment.
The issue is that zcalloc exists but zmalloc and zfree have been removed, which still works from a technical perspective, but is inconsistent.
How about adding zalloc and zfree as static functions that take struct ZAllocator allocator, like zcalloc?
| @@ -2,21 +2,16 @@ | |||
| #include <stdlib.h> | |||
There was a problem hiding this comment.
Can we remove this now that we're no longer referring to malloc and free in this file?
There was a problem hiding this comment.
the ZDEFAULT_ALLOCATOR in the header still is defined as:
#define ZDEFAULT_ALLOCATOR ((struct ZAllocator) { .alloc = malloc, .free = free })2 participants
Fixes #1
I've updated the API to support adding custom allocators via a
struct ZAllocatortype. I've also ran the test script provided.I've updated the examples and readme to reflect the changes and I also went in and added
-std=c99to gcc to ensure that no gcc extensions were being used.