git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Rose via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff Hostetler <git@jeffhostetler.com>,
	Seija <doremylover123@gmail.com>
Subject: Re: [PATCH v4] maintenance: use calloc instead of malloc where possible
Date: Mon, 05 Dec 2022 21:03:16 -0800	[thread overview]
Message-ID: <xmqqpmcxqg2z.fsf@gitster.g> (raw)
In-Reply-To: <pull.1390.v4.git.git.1670256724311.gitgitgadget@gmail.com> (Rose via GitGitGadget's message of "Mon, 05 Dec 2022 16:12:04 +0000")

"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.

  reply	other threads:[~2022-12-06  5:03 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-05 14:48 [PATCH] maintenance: use xcalloc instead of xmalloc where possible Rose via GitGitGadget
2022-12-05 15:01 ` Jeff Hostetler
2022-12-05 15:33 ` [PATCH v2] " Rose via GitGitGadget
2022-12-05 16:01   ` [PATCH v3] " Rose via GitGitGadget
2022-12-05 16:12     ` [PATCH v4] maintenance: use calloc instead of malloc " Rose via GitGitGadget
2022-12-06  5:03       ` Junio C Hamano [this message]
2022-12-06 17:38       ` [PATCH v5] revision: " Rose via GitGitGadget
2022-12-06 19:44         ` Ævar Arnfjörð Bjarmason
2022-12-06 19:53         ` [PATCH v6] " Rose via GitGitGadget
2022-12-07  8:44           ` Bagas Sanjaya

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqpmcxqg2z.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=doremylover123@gmail.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).