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

From: Jeff King <peff@peff.net>
To: 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: Tue, 12 Jul 2022 03:19:22 -0400	[thread overview]
Message-ID: <Ys0gerokoiLC9llA@coredump.intra.peff.net> (raw)
In-Reply-To: <bb1c85e4-50be-04a5-3c9f-f4c4dab6a50e@gmail.com>

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.

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.

-Peff