-
Notifications
You must be signed in to change notification settings - Fork 30
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 custom memory functions #159
base: master
Are you sure you want to change the base?
Conversation
8b2ce1b
to
5685b12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, looks really promising! I've left a couple of nitpicks regarding naming, and some overall suggestions. Also, it seems this PR was built on top of master
, please rebase it to dev
and then have it also point to dev
instead.
* - change CARRAY_INITIAL_SIZE from 5 to 4 | ||
* - change CARRAY_RESIZE to doubling arrays to reduce realloc calls | ||
* - remove calloc() from __carray_init(), expect user to allocate it | ||
* - remove ccord_calloc() from __carray_init(), expect user to allocate it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a third-party library so I don't think it's a good idea to change the original file as we'll no longer be able to keep it in sync with the original. Its best to open a PR in the jsmn-find
repo's to allow changing the default allocators, and then we can update it here
* returned pointer **must** be suitably aligned for any type. If @p | ||
* length is 0 or exceeds `PTRDIFF_MAX`, the behavior is undefined. | ||
*/ | ||
typedef void *(*malloc_fn_t)(size_t length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: To keep it consistent to other types in this repo it's best to miss the _t
suffix, also add a ccord_
prefix to all of those!
extern bool ccord_mem_is_initialized; | ||
|
||
/** @brief Initialize the memory functions */ | ||
void mem_init(malloc_fn_t malloc_fn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: rename to ccord_mem_init
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to namespace it to mem
instead of ccord
for consistency with everything else in core
. The ccord_
stuff above is inconsistent with that. I'll update it.
#include <stddef.h> | ||
#include <stdbool.h> | ||
|
||
#ifdef LOG_DEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new directive you added? This might be confused with log.c
's LOG_DEBUG
enum value. Could this be renamed to CCORD_DEBUG_MEM
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misunderstood the meaning of LOG_DEBUG. Will add a custom macro.
@@ -66,6 +86,75 @@ ccord_global_init() | |||
return CCORD_OK; | |||
} | |||
|
|||
static void * | |||
malloc_fn_system(size_t length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ccord_
prefix for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the purpose of that? It's static
and only used in this TU. I see no reason to namespace it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main idea here is prefixing with _
, whether its _mem
or _ccord
It's static and only used in this TU.
- We can quickly infer if a function is static at a glance by simply checking if it's prefixed by a
_
(no need to check its declaration) - That's how it is in the overall codebase, it's good to keep things consistent even if only for consistency's sake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright.
} | ||
|
||
static void * | ||
calloc_fn_system(size_t nmemb, size_t length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
} | ||
|
||
static void * | ||
realloc_fn_system(void *old_ptr, size_t length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
} | ||
|
||
static void | ||
free_fn_system(void *ptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
} | ||
|
||
static char * | ||
strdup_fn_system(const char *str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Closes #157
to test it before making changes to Concord.
Testing?