Re: [PATCH v4] maintenance: use calloc instead of malloc where possible

[Junio C Hamano <gitster@pobox.com>]

"Rose via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Seija <doremylover123@gmail.com>
>
> We can avoid having to call memset by calling calloc directly

What is not explained here is how that makes the code better in what
way.  Reduced line count?  Reduced cycle count?  Reduced line count?

The reason I ask is because the patch touches codepaths that do not
call memset() after malloc(), which the above may explain.

Also, the patch does not use calloc() directly, but uses
CALLOC_ARRAY() doesn't it?

>      +    maintenance: use calloc instead of malloc where possible

This is not about "git maintenance", is it?  Why is that subsystem
specifically named here?

This is not about the patch, because Signed-off-by: line has a legal
meaning (also see SubmittingPatches[[real-name]]), would you mind
explaining what is going on with your name?  The e-mailed patches
come from "Rose", but the patch author identifies themselves as
"Seija".

> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
> index ecd49ca268f..0e184bb5212 100644
> --- a/builtin/pack-redundant.c
> +++ b/builtin/pack-redundant.c
> @@ -51,7 +51,7 @@ 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]);

This is unrelated change, isn't it?

> @@ -61,9 +61,7 @@ static inline struct llist_item *llist_item_get(void)
>  
>  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);
>  }

I am somewhat torn on this one.

The original makes it crystal clear that the initial state of the
llist data structure is that the .front and the .back pointers that
point at the head and the tail of a linearly linked list point at
NULL and the .size member indicates there is zero elements on the
list.  IOW, unlike a mindless "memset()" in a

	x = malloc(sizeof(...));
	memset(x, '\0', sizeof(...));

sequence, the way members are cleared was meaningful in the
original.

Using CALLOC_ARRAY() makes it as bad as use of memset() that blindly
fills the memory reason with NUL bytes.  Surely a NULL pointer may
have the same bit representation as a region of memory filled with
NUL, and a size_t integer whose value is zero may also share the
same bit representation, but it lost clarity of the original.

On the other hand, if we are willing to accept the conciseness, and
accept the "\0 filled memory region is the naturally initialized
state for this structure" convention, then I do not see the value of
having a separate llist_init() helper function.  It is used only in
a handful places in this single file, so getting rid of the helper
and writing CALLOC_ARRAY(x, 1) to clear each instance of the structure
it is used to clear may not be a bad thing.

And the presented solution is neither.  Again, I'd prefer to keep
the original, but if we must use CALLOC_ARRAY() to replace it, then
I'd prefer to see that the helper function to be removed.

> 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);

This IS an improvement.  We do the mindless clearing either way, it
is just shorter and easier to follow.

> 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);

Ditto.

> @@ -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);

Ditto.

> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 9e36f24875d..c19bc441a96 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -98,12 +98,10 @@ 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;

I am somewhat negative on this for the same reason why I'd prefer to
keep the llist thing intact.  Also xdiff code is a borrowed code and
I'd rather see us not to touch it unnecessarily.

>  		if (cha->tail)
>  			cha->tail->next = ancur;
>  		if (!cha->head)
>
> base-commit: 805265fcf7a737664a8321aaf4a0587b78435184

Thanks.