Re: [PATCH v2 4/4] xdiff: introduce XDL_ALLOC_GROW()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 8, 2022, 22:17 UTC
Message-ID: <220709.86v8s78chj.gmgdl@evledraar.gmail.com>
In-Reply-To: <8c24cd7737b29d461788b71f6a94eb74c468ad33.1657297520.git.gitgitgadget@gmail.com>

On Fri, Jul 08 2022, Phillip Wood via GitGitGadget wrote:

Show 23 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Add a helper to grow an array. This is analogous to ALLOC_GROW() in
> the rest of the codebase but returns −1 on allocation failure to
> accommodate other users of libxdiff such as libgit2. 
> [...]
> +		if (XDL_ALLOC_GROW(cf->rcrecs, cf->count, cf->alloc))
> +void* xdl_alloc_grow_helper(void *p, long nr, long *alloc, size_t size)
> +{
> +	void *tmp = NULL;
> +	size_t n = ((LONG_MAX - 16) / 2 >= *alloc) ? 2 * *alloc + 16 : LONG_MAX;
> +	if (nr > n)
> +		n = nr;
> +	if (SIZE_MAX / size >= n)
> +		tmp = xdl_realloc(p, n * size);
> +	if (tmp) {
> +		*alloc = n;
> +	} else {
> +		xdl_free(p);
> +		*alloc = 0;
> +	}
> +	return tmp;
> +}

Whether you agree with the entire approach in my series-on-top[1] or not (and it looks like not), this way of doing it seems needlessly complex.

I.e. the whole "pass the size" business is here because you're wanting to use this as an expression, which ALLOC_GROW() isn't able to do.

But you've also changed every single callsite anyway.

So why not just change:

    if (XDL_ALLOC_GROW(recs, ...))

To:

    XDL_ALLOC_GROW(recs, ...);
    if (!recs)

And do away with needing to pass this through a function where we get the size, and thus losing the type information before we can call sizeof().

Then, as that series shows the implementation here could be pretty much an exact line-for-line copy of what we have in cache.h, including the same alloc_nr(), all without casts etc.

All of which seems much simpler than trying to maintain the constraint that this must be usable in an expression.

Your commit message sort-of addresses this by mentioning that this "returns −1 on allocation failure to accommodate other users of libxdiff such as libgit2".

But since libgit2 won't use this, but only a copy of this xdiff code in
libgit2 I don't see how that makes sense. We're only talking about using
this in the xdiff code we maintain, and even if libgit2 wanted to use it
it could deal with not being able to use it in an expression, no?
    	
1. https://lore.kernel.org/git/patch-5.7-3665576576f-20220708T140354Z-avarab@gmail.com/