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