Re: [PATCH 2/2] use CALLOC_ARRAY

["René Scharfe." <l.s.r@web.de>]

Am 15.03.21 um 20:26 schrieb Junio C Hamano:
> https://github.com/git/git/runs/2114848725?check_suite_focus=true
> tells us that we want to be using CALLOC_ARRAY() instead of
> xcalloc() at these places.
>
> Quite a many are single-element allocations that want NUL-filled
> piece of memory.  We may want to resurrect the "one-element calloc
> is exempt" thing (in other words, xcalloc(1, size_of_one_element) is
> perfectly fine, even though xcalloc(size_of_one_element, 1) must
> swap the order of arguments).

Leaving the idiomatic use of xcalloc for allocating a single cleared
object alone can make sense.  CALLOC_ARRAY with a single element
might become idiomatic over time as well, though, but with "C" (for
continuous) and "ARRAY" having two name parts screaming for multiple
elements might be a bit much.

> But some are true multi-element allocations that we may want to fix
> in each topic in flight.
>
> compat/simple-ipc/ipc-unix-socket.c::836
>     1e7cdcb6 (simple-ipc: add Unix domain socket implementation, 2021-03-09)
>
> merge-strategies.c::498
>     72a74644 (merge-octopus: rewrite in C, 2020-11-24)
>
> t/helper/test-bloom.c::72
>     f1294eaf (bloom.c: introduce core Bloom filter constructs, 2020-03-30)
>
>
> Also there are a few multi-element allocations that are old, which
> weren't somehow caught by the patch I am responding to.

When I run coccicheck agains seen it still misses the following
conversions:

 builtin/checkout.c                  |    2 +-
 builtin/log.c                       |    4 ++--
 builtin/receive-pack.c              |   10 ++++------
 t/helper/test-bloom.c               |    2 +-

That's with spatch built from the latest Coccinelle source as of March
2nd.  Strange.

>
> builtin/receive-pack.c::2351
>     0a1bc12b (receive-pack: allow pushes that update .git/shallow, 2013-12-05)
>
> ----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 -----
>
> GIT_VERSION = 2.31.0.rc2.390.g46dd778bfd
>     SPATCH contrib/coccinelle/hashmap.cocci
>     SPATCH contrib/coccinelle/commit.cocci
>     SPATCH contrib/coccinelle/xcalloc.cocci
>     SPATCH contrib/coccinelle/preincr.cocci
>     SPATCH contrib/coccinelle/free.cocci
>     SPATCH contrib/coccinelle/qsort.cocci
>     SPATCH contrib/coccinelle/object_id.cocci
>     SPATCH contrib/coccinelle/xstrdup_or_null.cocci
>     SPATCH contrib/coccinelle/swap.cocci
>     SPATCH contrib/coccinelle/strbuf.cocci
>     SPATCH contrib/coccinelle/flex_alloc.cocci
>     SPATCH contrib/coccinelle/array.cocci
>      SPATCH result: contrib/coccinelle/array.cocci.patch
> + set +x
> Coccinelle suggests the following changes in 'contrib/coccinelle/array.cocci.patch':
> diff -u -p a/builtin/checkout.c b/builtin/checkout.c
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -741,7 +741,7 @@ static int merge_working_tree(const stru
>  				       &new_branch_info->commit->object.oid :
>  				       &new_branch_info->oid, NULL);
>  		if (opts->overwrite_ignore) {
> -			topts.dir = xcalloc(1, sizeof(*topts.dir));
> +			CALLOC_ARRAY(topts.dir, 1);
>  			topts.dir->flags |= DIR_SHOW_IGNORED;
>  			setup_standard_excludes(topts.dir);
>  		}
> diff -u -p a/builtin/log.c b/builtin/log.c
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -230,7 +230,7 @@ static void cmd_log_init_finish(int argc
>  	}
>
>  	if (mailmap) {
> -		rev->mailmap = xcalloc(1, sizeof(struct string_list));
> +		CALLOC_ARRAY(rev->mailmap, 1);
>  		read_mailmap(rev->mailmap);
>  	}
>
> @@ -2141,7 +2141,7 @@ int cmd_format_patch(int argc, const cha
>  	}
>
>  	if (in_reply_to || thread || cover_letter)
> -		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
> +		CALLOC_ARRAY(rev.ref_message_ids, 1);
>  	if (in_reply_to) {
>  		const char *msgid = clean_message_id(in_reply_to);
>  		string_list_append(rev.ref_message_ids, msgid);
> diff -u -p a/builtin/receive-pack.c b/builtin/receive-pack.c
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1040,7 +1040,7 @@ static int read_proc_receive_report(stru
>  					report = hint->report;
>  					while (report->next)
>  						report = report->next;
> -					report->next = xcalloc(1, sizeof(struct ref_push_report));
> +					CALLOC_ARRAY(report->next, 1);
>  					report = report->next;
>  				}
>  				new_report = 0;
> @@ -2348,11 +2348,9 @@ static void prepare_shallow_update(struc
>  	ALLOC_ARRAY(si->used_shallow, si->shallow->nr);
>  	assign_shallow_commits_to_refs(si, si->used_shallow, NULL);
>
> -	si->need_reachability_test =
> -		xcalloc(si->shallow->nr, sizeof(*si->need_reachability_test));
> -	si->reachable =
> -		xcalloc(si->shallow->nr, sizeof(*si->reachable));
> -	si->shallow_ref = xcalloc(si->ref->nr, sizeof(*si->shallow_ref));
> +	CALLOC_ARRAY(si->need_reachability_test, si->shallow->nr);
> +	CALLOC_ARRAY(si->reachable, si->shallow->nr);
> +	CALLOC_ARRAY(si->shallow_ref, si->ref->nr);
>
>  	for (i = 0; i < si->nr_ours; i++)
>  		si->need_reachability_test[si->ours[i]] = 1;
> diff -u -p a/builtin/repack.c b/builtin/repack.c
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -327,7 +327,7 @@ static void init_pack_geometry(struct pa
>  	struct packed_git *p;
>  	struct pack_geometry *geometry;
>
> -	*geometry_p = xcalloc(1, sizeof(struct pack_geometry));
> +	CALLOC_ARRAY(*geometry_p, 1);
>  	geometry = *geometry_p;
>
>  	for (p = get_all_packs(the_repository); p; p = p->next) {
> diff -u -p a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c
> --- a/compat/simple-ipc/ipc-unix-socket.c
> +++ b/compat/simple-ipc/ipc-unix-socket.c
> @@ -130,7 +130,7 @@ enum ipc_active_state ipc_client_try_con
>  	trace2_region_leave("ipc-client", "try-connect", NULL);
>
>  	if (state == IPC_STATE__LISTENING) {
> -		(*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection));
> +		CALLOC_ARRAY((*p_connection), 1);
>  		(*p_connection)->fd = fd;
>  	}
>
> @@ -819,7 +819,7 @@ int ipc_server_run_async(struct ipc_serv
>  		return ret;
>  	}
>
> -	server_data = xcalloc(1, sizeof(*server_data));
> +	CALLOC_ARRAY(server_data, 1);
>  	server_data->magic = MAGIC_SERVER_DATA;
>  	server_data->application_cb = application_cb;
>  	server_data->application_data = application_data;
> @@ -833,11 +833,9 @@ int ipc_server_run_async(struct ipc_serv
>  	pthread_cond_init(&server_data->work_available_cond, NULL);
>
>  	server_data->queue_size = nr_threads * FIFO_SCALE;
> -	server_data->fifo_fds = xcalloc(server_data->queue_size,
> -					sizeof(*server_data->fifo_fds));
> +	CALLOC_ARRAY(server_data->fifo_fds, server_data->queue_size);
>
> -	server_data->accept_thread =
> -		xcalloc(1, sizeof(*server_data->accept_thread));
> +	CALLOC_ARRAY(server_data->accept_thread, 1);
>  	server_data->accept_thread->magic = MAGIC_ACCEPT_THREAD_DATA;
>  	server_data->accept_thread->server_data = server_data;
>  	server_data->accept_thread->server_socket = server_socket;
> @@ -851,7 +849,7 @@ int ipc_server_run_async(struct ipc_serv
>  	for (k = 0; k < nr_threads; k++) {
>  		struct ipc_worker_thread_data *wtd;
>
> -		wtd = xcalloc(1, sizeof(*wtd));
> +		CALLOC_ARRAY(wtd, 1);
>  		wtd->magic = MAGIC_WORKER_THREAD_DATA;
>  		wtd->server_data = server_data;
>
> diff -u -p a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
> --- a/compat/simple-ipc/ipc-win32.c
> +++ b/compat/simple-ipc/ipc-win32.c
> @@ -184,7 +184,7 @@ enum ipc_active_state ipc_client_try_con
>  	trace2_region_leave("ipc-client", "try-connect", NULL);
>
>  	if (state == IPC_STATE__LISTENING) {
> -		(*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection));
> +		CALLOC_ARRAY((*p_connection), 1);
>  		(*p_connection)->fd = fd;
>  	}
>
> @@ -625,7 +625,7 @@ int ipc_server_run_async(struct ipc_serv
>  		return -2;
>  	}
>
> -	server_data = xcalloc(1, sizeof(*server_data));
> +	CALLOC_ARRAY(server_data, 1);
>  	server_data->magic = MAGIC_SERVER_DATA;
>  	server_data->application_cb = application_cb;
>  	server_data->application_data = application_data;
> @@ -640,7 +640,7 @@ int ipc_server_run_async(struct ipc_serv
>  	for (k = 0; k < nr_threads; k++) {
>  		struct ipc_server_thread_data *std;
>
> -		std = xcalloc(1, sizeof(*std));
> +		CALLOC_ARRAY(std, 1);
>  		std->magic = MAGIC_SERVER_THREAD_DATA;
>  		std->server_data = server_data;
>  		std->hPipe = INVALID_HANDLE_VALUE;
> diff -u -p a/merge-ort.c b/merge-ort.c
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3439,7 +3439,7 @@ static void merge_start(struct merge_opt
>  		trace2_region_leave("merge", "allocate/init", opt->repo);
>  		return;
>  	}
> -	opt->priv = xcalloc(1, sizeof(*opt->priv));
> +	CALLOC_ARRAY(opt->priv, 1);
>
>  	/* Initialization of various renames fields */
>  	renames = &opt->priv->renames;
> diff -u -p a/merge-strategies.c b/merge-strategies.c
> --- a/merge-strategies.c
> +++ b/merge-strategies.c
> @@ -495,8 +495,7 @@ int merge_strategies_octopus(struct repo
>  		return 2;
>  	}
>
> -	reference_commit = xcalloc(commit_list_count(remotes) + 1,
> -				   sizeof(struct commit *));
> +	CALLOC_ARRAY(reference_commit, commit_list_count(remotes) + 1);
>  	reference_commit[0] = head_commit;
>  	reference_tree = head_tree;
>
> diff -u -p a/t/helper/test-bloom.c b/t/helper/test-bloom.c
> --- a/t/helper/test-bloom.c
> +++ b/t/helper/test-bloom.c
> @@ -69,7 +69,7 @@ int cmd__bloom(int argc, const char **ar
>  		struct bloom_filter filter;
>  		int i = 2;
>  		filter.len =  (settings.bits_per_entry + BITS_PER_WORD - 1) / BITS_PER_WORD;
> -		filter.data = xcalloc(filter.len, sizeof(unsigned char));
> +		CALLOC_ARRAY(filter.data, filter.len);
>
>  		if (argc - 1 < i)
>  			usage(bloom_usage);
> diff -u -p a/unix-stream-server.c b/unix-stream-server.c
> --- a/unix-stream-server.c
> +++ b/unix-stream-server.c
> @@ -72,7 +72,7 @@ int unix_stream_server__create(
>  		return -1;
>  	}
>
> -	server_socket = xcalloc(1, sizeof(*server_socket));
> +	CALLOC_ARRAY(server_socket, 1);
>  	server_socket->path_socket = strdup(path);
>  	server_socket->fd_socket = fd_socket;
>  	lstat(path, &server_socket->st_socket);
> error: Coccinelle suggested some changes
> Error: Process completed with exit code 1.
>