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.
XDL_ALLOC_GROW is defined as
+/*
+ * Ensure array p can accommodate at least nr elements, growing the
+ * array and updating alloc (which is the number of allocated
+ * elements) as necessary. Frees p and returns -1 on failure, returns
+ * 0 on success
+ */
+#define XDL_ALLOC_GROW(p, nr, alloc) \
+ (-!((nr) <= (alloc) || \
+ ((p) = xdl_alloc_grow_helper((p), (nr), &(alloc), sizeof(*(p))))))
+
so it returns -1 if xdl_alloc_grow_helper() returns NULL, which it does
if there is an allocation failure or the multiplication overflows.
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?
I'm not relying on the return value of xrealloc() in the macro
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.
I'm not quite sure what you're saying in these last three paragraphs
Best Wishes
Phillip
