git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Matheus Tavares <matheus.bernardino@usp.br>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH 2/3] mem-pool: use more standard initialization and finalization
Date: Thu, 13 Aug 2020 21:38:04 -0700	[thread overview]
Message-ID: <xmqqpn7tzvtf.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <f13a52055cd975d457e0593cbabb70897e78024b.1597374135.git.gitgitgadget@gmail.com> (Elijah Newren via GitGitGadget's message of "Fri, 14 Aug 2020 03:02:14 +0000")

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

> written.  mem_pool_init() does essentially the following (simplified
> for purposes of explanation here):
>
>     void mem_pool_init(struct mem_pool **pool...)
>     {
>         *pool = xcalloc(1, sizeof(*pool));
>
> It seems weird to require that mem_pools can only be accessed through a
> pointer.

Yup, if the _init() were to also allocate, I would expect it to be
more like

	struct mem_pool *mem_pool_create(...)
	{
		struct mem_pool *pool = xcalloc(1, sizeof(*pool));
		...
		return pool;
	}

It also is OK to let the caller pass uninitialized region of memory,
which is how we usually arrange _init() to work.  It seems that that
is the approach this patch takes.

> -void mem_pool_init(struct mem_pool **mem_pool, size_t initial_size)
> +void mem_pool_init(struct mem_pool *mem_pool, size_t initial_size)
>  {
> -	struct mem_pool *pool;
> -
> -	if (*mem_pool)
> -		return;
> -
> -	pool = xcalloc(1, sizeof(*pool));
> -
> -	pool->block_alloc = BLOCK_GROWTH_SIZE;
> +	mem_pool->mp_block = NULL;
> +	mem_pool->pool_alloc = 0;
> +	mem_pool->block_alloc = BLOCK_GROWTH_SIZE;
>  
>  	if (initial_size > 0)
> -		mem_pool_alloc_block(pool, initial_size, NULL);
> -
> -	*mem_pool = pool;
> +		mem_pool_alloc_block(mem_pool, initial_size, NULL);

It used to be that this function both knew and took control of all
the bits in *pool memory by using xcalloc().  Any field the function
assigned to of course got explicitly the value the function wanted
it to have, and all other fields were left to 0.

It may happen to be still the case (i.e. the assignments we see in
this function cover all the fields defined), but don't we need some
provision to make sure it will hold to be true in the future?

Starting it with "memset(pool, 0, sizeof(*pool)" would be one way.

You'd standardize to s/mem_pool/pool/ in [3/3]; shouldn't this be
written with pool to begin with, instead of reintroducing mem_pool
that is of different type from the original?

> -	if (!*pool_ptr)
> -		mem_pool_init(pool_ptr, 0);
> +	if (!*pool_ptr) {
> +		*pool_ptr = xmalloc(sizeof(**pool_ptr));
> +		mem_pool_init(*pool_ptr, 0);

This one gives an uninitialized chunk of memory to the _init(); an
example of the caller that the earlier comment may matter.

> +	istate->ce_mem_pool = xmalloc(sizeof(*istate->ce_mem_pool));
>  	if (istate->version == 4) {
> -		mem_pool_init(&istate->ce_mem_pool,
> +		mem_pool_init(istate->ce_mem_pool,
>  				estimate_cache_size_from_compressed(istate->cache_nr));
>  	} else {
> -		mem_pool_init(&istate->ce_mem_pool,
> +		mem_pool_init(istate->ce_mem_pool,
>  				estimate_cache_size(mmap_size, istate->cache_nr));
>  	}

Likewise.

Thanks.

  reply	other threads:[~2020-08-14  4:38 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-14  3:02 [PATCH 0/3] Extend and add a little more generalization to the mem_pool API Elijah Newren via GitGitGadget
2020-08-14  3:02 ` [PATCH 1/3] mem-pool: add convenience functions for xstrdup and xstrndup Elijah Newren via GitGitGadget
2020-08-14  4:42   ` Eric Sunshine
2020-08-14  3:02 ` [PATCH 2/3] mem-pool: use more standard initialization and finalization Elijah Newren via GitGitGadget
2020-08-14  4:38   ` Junio C Hamano [this message]
2020-08-14  3:02 ` [PATCH 3/3] mem-pool: use consistent pool variable name Elijah Newren via GitGitGadget
2020-08-14  3:51 ` [PATCH 0/3] Extend and add a little more generalization to the mem_pool API Matheus Tavares Bernardino
2020-08-14  6:00 ` [PATCH v2 " Elijah Newren via GitGitGadget
2020-08-14  6:00   ` [PATCH v2 1/3] mem-pool: add convenience functions for xstrdup and xstrndup Elijah Newren via GitGitGadget
2020-08-14  8:21     ` René Scharfe
2020-08-14  6:00   ` [PATCH v2 2/3] mem-pool: use more standard initialization and finalization Elijah Newren via GitGitGadget
2020-08-14  6:00   ` [PATCH v2 3/3] mem-pool: use consistent pool variable name Elijah Newren via GitGitGadget
2020-08-15 17:37   ` [PATCH v3 0/3] Extend and add a little more generalization to the mem_pool API Elijah Newren via GitGitGadget
2020-08-15 17:37     ` [PATCH v3 1/3] mem-pool: add convenience functions for strdup and strndup Elijah Newren via GitGitGadget
2020-08-15 17:37     ` [PATCH v3 2/3] mem-pool: use more standard initialization and finalization Elijah Newren via GitGitGadget
2020-08-15 17:37     ` [PATCH v3 3/3] mem-pool: use consistent pool variable name Elijah Newren via GitGitGadget
2020-08-18 19:27     ` [PATCH v3 0/3] Extend and add a little more generalization to the mem_pool API Junio C Hamano

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=xmqqpn7tzvtf.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=matheus.bernardino@usp.br \
    --cc=newren@gmail.com \
    --subject='Re: [PATCH 2/3] mem-pool: use more standard initialization and finalization' \
    /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

Code repositories for project(s) associated with this 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).