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

[Phillip Wood <phillip.wood123@gmail.com>]

On 08/07/2022 23:17, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jul 08 2022, Phillip Wood via GitGitGadget wrote:
> 
>> 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)

Because I think it's ugly, we'd never define a function as void(int* 
result, args...) and I don't think we should do that for a macro where 
we can avoid it. The existing ALLOC_* macros make sense as statements 
because they die on failure.

Best Wishes

Phillip

> 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/