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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH 3/3] xdiff: introduce XDL_ALLOC_GROW()
Date: Thu, 30 Jun 2022 12:54:23 +0200	[thread overview]
Message-ID: <220630.86czeqe74c.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <da996677f0730ec7a50d399524fb58c44dad468a.1656516334.git.gitgitgadget@gmail.com>

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.

And those are defined as:

	#define xdl_malloc(x) xmalloc(x)
	#define xdl_free(ptr) free(ptr)
	#define xdl_realloc(ptr,x) xrealloc(ptr,x)

And for e.g. xmalloc() we do:

        return do_xmalloc(size, 0);

Where that 0=gently, i.e. we'll die() instead of error(), the xrealloc()
then has no "gently" option.

Is the (pretty glaring, if that's the case) unstated assumption here
that this doesn't in fact return -1 on allocation failure, but you're
expected to replace the underlying xmalloc() with an implementation that
does?

If so I'm doubly confused by this, since you're providing alternatives
to e.g.:

	#define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))

So if that's the plan why would we need an XDL_ALLOC_ARRAY(), can't you
just check that it returns non-NULL?

That's not possible with our current xmalloc(), but ditto for this
series, and apparently the compiler isn't smart enough to yell at you
about that...

I wondered if we were just missing the returns_nonnull attribute, but
playing around with it I couldn't get GCC at least to warn about
checking xmalloc()'s return value for non-NULL.