On Tue, 1 May 2018 14:34:03 -0700
Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> +void *allocate_alloc_state(void)
> +{
> + return xcalloc(1, sizeof(struct alloc_state));
> +}
> +
> +void clear_alloc_state(struct alloc_state *s)
> +{
> + while (s->slab_nr > 0) {
> + s->slab_nr--;
> + free(s->slabs[s->slab_nr]);
> + }
> +}
These functions are asymmetrical. I understand why it is this way
(because we use pointers, and we want to use FREE_AND_NULL), but would
still prefer _INIT and _release().
> static inline void *alloc_node(struct alloc_state *s, size_t node_size)
> {
> void *ret;
> @@ -45,54 +63,56 @@ static inline void *alloc_node(struct alloc_state *s, size_t node_size)
> ret = s->p;
> s->p = (char *)s->p + node_size;
> memset(ret, 0, node_size);
> +
> + ALLOC_GROW(s->slabs, s->slab_nr + 1, s->slab_alloc);
> + s->slabs[s->slab_nr++] = ret;
> +
> return ret;
> }
This unconditionally grows the slabs for each node allocation. Shouldn't
more than one node fit in each slab?
> +extern struct alloc_state the_repository_blob_state;
> +extern struct alloc_state the_repository_tree_state;
> +extern struct alloc_state the_repository_commit_state;
> +extern struct alloc_state the_repository_tag_state;
> +extern struct alloc_state the_repository_object_state;
(Context: these were in alloc.h)
These seem to be used only in object.c, and only in one function - could
we declare them "static" inside that function instead?
> -struct object_parser *object_parser_new(void)
> +struct object_parser *object_parser_new(int is_the_repo)
> {
> struct object_parser *o = xmalloc(sizeof(*o));
> memset(o, 0, sizeof(*o));
> +
> + if (is_the_repo) {
> + o->blob_state = &the_repository_blob_state;
> + o->tree_state = &the_repository_tree_state;
> + o->commit_state = &the_repository_commit_state;
> + o->tag_state = &the_repository_tag_state;
> + o->object_state = &the_repository_object_state;
> + } else {
> + o->blob_state = allocate_alloc_state();
> + o->tree_state = allocate_alloc_state();
> + o->commit_state = allocate_alloc_state();
> + o->tag_state = allocate_alloc_state();
> + o->object_state = allocate_alloc_state();
> + }
> return o;
> }
I don't think saving 5 allocations is worth the code complexity (of the
additional parameter).
