Re: [PATCH 3/3] xdiff: introduce XDL_ALLOC_GROW()

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Phillip Wood <phillip.wood123@gmail.com>
Cc: phillip.wood@dunelm.org.uk,
	Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 3/3] xdiff: introduce XDL_ALLOC_GROW()
Date: Thu, 30 Jun 2022 15:25:46 +0200	[thread overview]
Message-ID: <220630.86zghuclzq.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <0a1650bf-a7f5-0cc5-e6c9-0e02d1f542bf@gmail.com>

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

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.