git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Dec 6, 2022, 05:03 UTC
Message-ID
<xmqqpmcxqg2z.fsf@gitster.g>
In-Reply-To
<pull.1390.v4.git.git.1670256724311.gitgitgadget@gmail.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".

Show 13 quoted lines
> 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?
Show 9 quoted lines
> @@ -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.

Show 13 quoted lines
> 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.

Show 18 quoted lines
> 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.
Show 11 quoted lines
> @@ -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.
Show 15 quoted lines
> 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.

Show 5 quoted lines
>  		if (cha->tail)
>  			cha->tail->next = ancur;
>  		if (!cha->head)
>
> base-commit: 805265fcf7a737664a8321aaf4a0587b78435184
Thanks.
Previous: Rose via GitGitGadgetNext: Rose via GitGitGadget
Message 6 of 16 in “maintenance: use xcalloc instead of xmalloc where possible”
  1. maintenance: use xcalloc instead of xmalloc where possibleRose via GitGitGadget, Dec 5, 2022
  2. Jeff HostetlerDec 5, 2022
  3. maintenance: use xcalloc instead of xmalloc where possibleRose via GitGitGadget, Dec 5, 2022
  4. maintenance: use xcalloc instead of xmalloc where possibleRose via GitGitGadget, Dec 5, 2022
  5. maintenance: use calloc instead of malloc where possibleRose via GitGitGadget, Dec 5, 2022
  6. Junio C HamanoDec 6, 2022
  7. revision: use calloc instead of malloc where possibleRose via GitGitGadget, Dec 6, 2022
  8. Ævar Arnfjörð BjarmasonDec 6, 2022
  9. revision: use calloc instead of malloc where possibleRose via GitGitGadget, Dec 6, 2022
  10. Bagas SanjayaDec 7, 2022
  11. revision: use calloc instead of malloc where possibleAreaZR via GitGitGadget, Dec 18, 2024
  12. git: use calloc instead of malloc where possibleAreaZR via GitGitGadget, Dec 18, 2024
  13. git: use calloc instead of malloc where possibleAreaZR via GitGitGadget, Dec 18, 2024
  14. git: use calloc instead of malloc + memset where possibleAreaZR via GitGitGadget, Dec 18, 2024
  15. Junio C HamanoDec 18, 2024
  16. git: use calloc instead of malloc + memset where possibleAreaZR via GitGitGadget, Dec 18, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.