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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 30, 2022, 13:25 UTC
Message-ID: <220630.86zghuclzq.gmgdl@evledraar.gmail.com>
In-Reply-To: <0a1650bf-a7f5-0cc5-e6c9-0e02d1f542bf@gmail.com>

On Thu, Jun 30 2022, Phillip Wood wrote:

Show 20 quoted lines

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