On Tue, Dec 06 2022, Rose via GitGitGadget wrote: > From: Seija <doremylover123@xxxxxxxxx> > > We can avoid having to call memset by calling calloc directly > > Signed-off-by: Seija doremylover123@xxxxxxxxx > --- > revision: use calloc instead of malloc where possible > > We can avoid having to call memset by calling calloc directly > > Signed-off-by: Seija doremylover123@xxxxxxxxx > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1390%2FAtariDreams%2Fcalloc-v5 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1390/AtariDreams/calloc-v5 > Pull-Request: https://github.com/git/git/pull/1390 > > Range-diff vs v4: > > 1: 3cd6b1eab13 ! 1: 8072fa30e4f maintenance: use calloc instead of malloc where possible > @@ Metadata > Author: Seija <doremylover123@xxxxxxxxx> > > ## Commit message ## > - maintenance: use calloc instead of malloc where possible > + revision: use calloc instead of malloc where possible > > We can avoid having to call memset by calling calloc directly > > @@ Commit message > > ## builtin/pack-redundant.c ## > @@ builtin/pack-redundant.c: static inline struct llist_item *llist_item_get(void) > - new_item = free_nodes; > - free_nodes = free_nodes->next; > - } else { > -- int i = 1; > -+ size_t i = 1; > - ALLOC_ARRAY(new_item, BLKSIZE); > - for (; i < BLKSIZE; i++) > - llist_item_put(&new_item[i]); > -@@ builtin/pack-redundant.c: static inline struct llist_item *llist_item_get(void) > + return new_item; > + } > > - static inline void llist_init(struct llist **list) > - { > +-static inline void llist_init(struct llist **list) > +-{ > - *list = xmalloc(sizeof(struct llist)); > - (*list)->front = (*list)->back = NULL; > - (*list)->size = 0; > -+ CALLOC_ARRAY(*list, 1); > - } > - > +-} > +- > static struct llist * llist_copy(struct llist *list) > + { > + struct llist *ret; > + struct llist_item *new_item, *old_item, *prev; > + > +- llist_init(&ret); > ++ CALLOC_ARRAY(ret, 1); > + > + if ((ret->size = list->size) == 0) > + return ret; > +@@ builtin/pack-redundant.c: static void load_all_objects(void) > + struct pack_list *pl = local_packs; > + struct llist_item *hint, *l; > + > +- llist_init(&all_objects); > ++ CALLOC_ARRAY(all_objects, 1); > + > + while (pl) { > + hint = NULL; > +@@ builtin/pack-redundant.c: static void cmp_local_packs(void) > + > + /* only one packfile */ > + if (!pl->next) { > +- llist_init(&pl->unique_objects); > ++ CALLOC_ARRAY(pl->unique_objects, 1); > + return; > + } > + > +@@ builtin/pack-redundant.c: static struct pack_list * add_pack(struct packed_git *p) > + return NULL; > + > + l.pack = p; > +- llist_init(&l.remaining_objects); > ++ CALLOC_ARRAY(l.remaining_objects, 1); > + > + if (open_pack_index(p)) > + return NULL; > +@@ builtin/pack-redundant.c: int cmd_pack_redundant(int argc, const char **argv, const char *prefix) > + scan_alt_odb_packs(); > + > + /* ignore objects given on stdin */ > +- llist_init(&ignore); > ++ CALLOC_ARRAY(ignore, 1); > + if (!isatty(0)) { > + while (fgets(buf, sizeof(buf), stdin)) { > + oid = xmalloc(sizeof(*oid)); > > ## remote.c ## > @@ remote.c: void apply_push_cas(struct push_cas_option *cas, > @@ submodule.c: static struct fetch_task *fetch_task_create(struct submodule_parall > > task->sub = submodule_from_path(spf->r, treeish_name, path); > > - > - ## xdiff/xutils.c ## > -@@ xdiff/xutils.c: void *xdl_cha_alloc(chastore_t *cha) { > - void *data; > - > - if (!(ancur = cha->ancur) || ancur->icurr == cha->nsize) { > -- if (!(ancur = (chanode_t *) xdl_malloc(sizeof(chanode_t) + cha->nsize))) { > -+ if (!(ancur = (chanode_t *) xdl_calloc(1, sizeof(chanode_t) + cha->nsize))) { > - > - return NULL; > - } > -- ancur->icurr = 0; > -- ancur->next = NULL; > - if (cha->tail) > - cha->tail->next = ancur; > - if (!cha->head) > > > builtin/pack-redundant.c | 17 +++++------------ > remote.c | 4 ++-- > submodule.c | 10 +++++----- > 3 files changed, 12 insertions(+), 19 deletions(-) > > diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c > index ecd49ca268f..ce5be807cf0 100644 > --- a/builtin/pack-redundant.c > +++ b/builtin/pack-redundant.c > @@ -59,19 +59,12 @@ static inline struct llist_item *llist_item_get(void) > return new_item; > } > > -static inline void llist_init(struct llist **list) > -{ > - *list = xmalloc(sizeof(struct llist)); > - (*list)->front = (*list)->back = NULL; > - (*list)->size = 0; > -} > - > static struct llist * llist_copy(struct llist *list) > { > struct llist *ret; > struct llist_item *new_item, *old_item, *prev; > > - llist_init(&ret); > + CALLOC_ARRAY(ret, 1); > > if ((ret->size = list->size) == 0) > return ret; > @@ -448,7 +441,7 @@ static void load_all_objects(void) > struct pack_list *pl = local_packs; > struct llist_item *hint, *l; > > - llist_init(&all_objects); > + CALLOC_ARRAY(all_objects, 1); > > while (pl) { > hint = NULL; > @@ -475,7 +468,7 @@ static void cmp_local_packs(void) > > /* only one packfile */ > if (!pl->next) { > - llist_init(&pl->unique_objects); > + CALLOC_ARRAY(pl->unique_objects, 1); > return; > } > > @@ -512,7 +505,7 @@ static struct pack_list * add_pack(struct packed_git *p) > return NULL; > > l.pack = p; > - llist_init(&l.remaining_objects); > + CALLOC_ARRAY(l.remaining_objects, 1); > > if (open_pack_index(p)) > return NULL; > @@ -620,7 +613,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix) > scan_alt_odb_packs(); > > /* ignore objects given on stdin */ > - llist_init(&ignore); > + CALLOC_ARRAY(ignore, 1); > if (!isatty(0)) { > while (fgets(buf, sizeof(buf), stdin)) { > oid = xmalloc(sizeof(*oid)); > diff --git a/remote.c b/remote.c > index 60869beebe7..475a1d18af0 100644 > --- a/remote.c > +++ b/remote.c > @@ -2741,9 +2741,9 @@ void apply_push_cas(struct push_cas_option *cas, > > struct remote_state *remote_state_new(void) > { > - struct remote_state *r = xmalloc(sizeof(*r)); > + struct remote_state *r; > > - memset(r, 0, sizeof(*r)); > + CALLOC_ARRAY(r, 1); > > hashmap_init(&r->remotes_hash, remotes_hash_cmp, NULL, 0); > hashmap_init(&r->branches_hash, branches_hash_cmp, NULL, 0); > diff --git a/submodule.c b/submodule.c > index 8ac2fca855d..015102a83d6 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1458,14 +1458,13 @@ struct fetch_task { > */ > static const struct submodule *get_non_gitmodules_submodule(const char *path) > { > - struct submodule *ret = NULL; > + struct submodule *ret; > const char *name = default_name_or_path(path); > > if (!name) > return NULL; > > - ret = xmalloc(sizeof(*ret)); > - memset(ret, 0, sizeof(*ret)); > + CALLOC_ARRAY(ret, 1); > ret->path = name; > ret->name = name; > > @@ -1504,8 +1503,9 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf > const char *path, > const struct object_id *treeish_name) > { > - struct fetch_task *task = xmalloc(sizeof(*task)); > - memset(task, 0, sizeof(*task)); > + struct fetch_task *task; > + > + CALLOC_ARRAY(task, 1); > > task->sub = submodule_from_path(spf->r, treeish_name, path); > > > base-commit: 2e71cbbddd64695d43383c25c7a054ac4ff86882 This is partially some of the sentiments Junio did in the v4 review (although I looked at this before reading that). I think calloc-ing or memset-ing a struct is going in the opposite direction of where we've been trending, which is to have explicit initializers. Sometimes it's justified, but in the "pack-redundant.c" case I think it's probably better & more future-proof to keep it, and get rid of the odd patter nof passing in a ** to an init function to have it alloc for us. I.e. this (there's some stray new free() in there from testing, sorry, but those are also bugs we should fix...): diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index ecd49ca268f..af91d643dd2 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -27,6 +27,7 @@ static struct llist { struct llist_item *back; size_t size; } *all_objects; /* all objects which must be present in local packfiles */ +#define LLIST_INIT { 0 } static struct pack_list { struct pack_list *next; @@ -59,19 +60,18 @@ static inline struct llist_item *llist_item_get(void) return new_item; } -static inline void llist_init(struct llist **list) +static inline void llist_init(struct llist *list) { - *list = xmalloc(sizeof(struct llist)); - (*list)->front = (*list)->back = NULL; - (*list)->size = 0; + struct llist blank = LLIST_INIT; + memcpy(list, &blank, sizeof(*list)); } static struct llist * llist_copy(struct llist *list) { - struct llist *ret; + struct llist *ret = xmalloc(sizeof(struct llist)); struct llist_item *new_item, *old_item, *prev; - llist_init(&ret); + llist_init(ret); if ((ret->size = list->size) == 0) return ret; @@ -420,6 +420,7 @@ static void minimize(struct pack_list **min) unique_pack_objects = llist_copy(all_objects); llist_sorted_difference_inplace(unique_pack_objects, missing); + free(missing); /* remove unique pack objects from the non_unique packs */ pl = non_unique; @@ -447,8 +448,9 @@ static void load_all_objects(void) { struct pack_list *pl = local_packs; struct llist_item *hint, *l; + all_objects = xmalloc(sizeof(struct llist)); - llist_init(&all_objects); + llist_init(all_objects); while (pl) { hint = NULL; @@ -475,7 +477,8 @@ static void cmp_local_packs(void) /* only one packfile */ if (!pl->next) { - llist_init(&pl->unique_objects); + pl->unique_objects = xmalloc(sizeof(struct llist)); + llist_init(pl->unique_objects); return; } @@ -512,7 +515,8 @@ static struct pack_list * add_pack(struct packed_git *p) return NULL; l.pack = p; - llist_init(&l.remaining_objects); + l.remaining_objects = xmalloc(sizeof(struct llist)); + llist_init(l.remaining_objects); if (open_pack_index(p)) return NULL; @@ -562,7 +566,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix) int i; int i_still_use_this = 0; struct pack_list *min = NULL, *red, *pl; - struct llist *ignore; + struct llist *ignore = xmalloc(sizeof(struct llist)); struct object_id *oid; char buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \n + \0 */ @@ -620,7 +624,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix) scan_alt_odb_packs(); /* ignore objects given on stdin */ - llist_init(&ignore); + llist_init(ignore); if (!isatty(0)) { while (fgets(buf, sizeof(buf), stdin)) { oid = xmalloc(sizeof(*oid)); @@ -635,6 +639,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix) llist_sorted_difference_inplace(pl->remaining_objects, ignore); pl = pl->next; } + free(ignore); cmp_local_packs(); Now, for: > @@ -1504,8 +1503,9 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf > const char *path, > const struct object_id *treeish_name) > { > - struct fetch_task *task = xmalloc(sizeof(*task)); > - memset(task, 0, sizeof(*task)); > + struct fetch_task *task; > + > + CALLOC_ARRAY(task, 1); > > task->sub = submodule_from_path(spf->r, treeish_name, path); This is no worse than before, but I think we're just converting here from one bad pattern to another. The "struct fetch_task" contains a "struct strvec", for many of our structs we play it fast and loose with whether you can calloc() it, but for "struct strbuf", "struct strvec" etc. we have an "empty_strvec" (or "slopbuf" etc.), so we *really* want those to be properly init'd. AFAICT we're just lucky that it happens to work with the strvec API in this case, but if this was a strbuf it could easily segfault etc. So the better fix here since we're spending review time on it isn't to move from one memset() equivalent pattern to another, but actually to start properly initializing this. Finally, this commit just seems to be all over the place in what it's changing. We have a bunch of: x = xmalloc(...); memset(x, 0, ...); In our tree, it's not clear why these are being picked out in particular, or what they have to do with each other. I think that if we're proposing to refactor these doing so with coccinelle is a much better thing to do. There's a parallel thread about that over at: https://lore.kernel.org/git/6694c52b38674859eb0390c7f62da1209a8d8ec3.1670266373.git.me@xxxxxxxxxxxx/
