From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 06 Jul 2022 13:23:32 GMT
Subject: Re: [PATCH 3/3] xdiff: introduce XDL_ALLOC_GROW()
Message-ID: <fa543f66-6e86-9f95-f164-20a68705cecd@gmail.com>
In-Reply-To: <220630.86zghuclzq.gmgdl@evledraar.gmail.com>

Hi Ævar

On 30/06/2022 14:25, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jun 30 2022, Phillip Wood wrote:
> 
>> On 30/06/2022 13:03, Phillip Wood wrote:
>>> On 30/06/2022 11:54, Ævar Arnfjörð Bjarmason wrote:
>>>>
>>>> On Wed, Jun 29 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.
>>>>
>>>> Urm, does it? I just skimmed this, so maybe I missed something, but I
>>>> don't see where you changed the definition of xdl_malloc(),
>>>> xdl_realloc() etc.
>>
>> Oh I think I might have misunderstood your question. For git.git it
>> will still die() but for other users that arrange for xdl_realloc() to
>> return NULL on failure it will return -1. The same applies to the
>> comments in the previous two patches about XDL_[CM]ALLOC_ARRAY()
>> returning NULL on allocation failure.
> 
> Yes, I meant that the "but returns −1 on allocation failure to
> accommodate other users of libxdiff such as libgit2" is really more of
> a:
> 
> 	...but *allows for* dropping in another xmalloc(), xrealloc()
> 	etc. implementation that doesn't die on failure.
> 
> So I think the rest of my upthread question still stands, i.e.:
> 
> 	"So if that's the plan why would we need an XDL_ALLOC_ARRAY(),
> 	can't you just check that it [I mean ALLOC_ARRAY()] returns
> 	non-NULL?"
> 
> I.e. if the plan is to replace the underlying x*() functions with
> non-fatal variants can't you use ALLOC_ARRAY() instead? I haven't tried
> that, but I don't see a reason we couldn't do that in principle...

As the cover letter says, the aim of this series is not to replace 
xmalloc() etc with non-fatal variants, it is just to make the xdiff code 
more readable. (One can already use a non-fatal allocation function for 
xdl_malloc()) I don't think that using ALLOC_ARRAY() in xdiff is helpful 
for other users as they would have to define their own array allocation 
macros, rather than just providing their own allocation functions. I 
would like to reduce the friction others have upstreaming xdiff patches 
to us, not increase it.

Best Wishes

Phillip

> Anyway, I'm all for the end goal here, but the way to get there seems to
> be a bit of an exercise in running with scissors the more I think about
> it.
> 
> I.e. the reason we're using these x*alloc() wrappers at all is because
> we're lazy and want to write stuff like:
> 
> 	struct stuff *foo = xmalloc(...);
> 	foo->bar = "baz";
> 
> Which the compiler is helpfully not yelling at us about, as opposed to
> doing the same with "malloc()", where it would spot the potential null
> pointer dereference.
> 
> (I'm using "compiler" here to be inclusive of extended gcc/clang options
> to detect this sort of thing, & other similar analyzers).
> 
> But re "scissors": if we're doing to be maintaining the xdiff code
> in-tree to be defined as our usual x*alloc() functions we're going to be
> carrying code that can't have test or analysis coverage.
> 
> Which I think brings me back to my suggestion of whether we can't just
> have non-fatal versions of these helper macros, define our own currently
> fatal versions in terms of those, and use the non-fatal versions in the
> xdiff/ code.
> 
>