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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Jeff King <peff@peff.net>, phillip.wood@dunelm.org.uk
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2 4/4] xdiff: introduce XDL_ALLOC_GROW()
Date: Wed, 13 Jul 2022 10:38:13 +0100	[thread overview]
Message-ID: <46f70aff-59f1-01ca-8f1f-b07b230011bc@gmail.com> (raw)
In-Reply-To: <Ys0gerokoiLC9llA@coredump.intra.peff.net>

Hi Peff

It's great to see you back on the list

On 12/07/2022 08:19, Jeff King wrote:
> On Mon, Jul 11, 2022 at 11:00:06AM +0100, Phillip Wood wrote:
> 
>>> 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.
> 
> Those macros are already unlike functions, though. They take a variable
> _name_, not a pointer to a variable, and assign to it. A version which
> looked like a function would have to be:
> 
>    if (XDL_ALLOC_GROW(&recs, ...))
> 
> So in my head I read them as assignment statements already, making the
> expression-like form a bit jarring.

I see what you're saying, I agree it is not exactly analogous. I tend to 
think of assignment as an expression because it returns the value that's 
being assigned, though here it's a bit muddy because the assignment is 
hidden inside the macro and there is no tell-tale '&' as there would be 
if it were calling function.

> Just my two cents. I don't care _too_ much either way, but I'd probably
> be swayed if one implementation is much simpler than the other.

I don't think there is much difference in the complexity in this case. 
In general I think that using a function with a macro wrapper can make 
the implementation more readable as the function it is not littered with 
the extra parentheses and backslashes that the same code in a macro 
requires.

Best Wishes

Phillip