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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 30, 2022, 12:03 UTC
Message-ID: <5734b393-57c8-dc63-282e-221ee1937351@gmail.com>
In-Reply-To: <220630.86czeqe74c.gmgdl@evledraar.gmail.com>

On 30/06/2022 11:54, Ævar Arnfjörð Bjarmason wrote:

Show 12 quoted lines

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

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.

Show 17 quoted lines

> 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

Show 7 quoted lines

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

Show 7 quoted lines

> 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