On 08/07/2022 23:17, Ævar Arnfjörð Bjarmason wrote:
On Fri, Jul 08 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.
[...]
+ if (XDL_ALLOC_GROW(cf->rcrecs, cf->count, cf->alloc))
+void* xdl_alloc_grow_helper(void *p, long nr, long *alloc, size_t size)
+{
+ void *tmp = NULL;
+ size_t n = ((LONG_MAX - 16) / 2 >= *alloc) ? 2 * *alloc + 16 : LONG_MAX;
+ if (nr > n)
+ n = nr;
+ if (SIZE_MAX / size >= n)
+ tmp = xdl_realloc(p, n * size);
+ if (tmp) {
+ *alloc = n;
+ } else {
+ xdl_free(p);
+ *alloc = 0;
+ }
+ return tmp;
+}
Whether you agree with the entire approach in my series-on-top[1] or not
(and it looks like not), this way of doing it seems needlessly complex.
I.e. the whole "pass the size" business is here because you're wanting
to use this as an expression, which ALLOC_GROW() isn't able to do.
But you've also changed every single callsite anyway.
So why not just change:
if (XDL_ALLOC_GROW(recs, ...))
To:
XDL_ALLOC_GROW(recs, ...);
if (!recs)
Because I think it's ugly, we'd never define a function as void(int*
result, args...) and I don't think we should do that for a macro where
we can avoid it. The existing ALLOC_* macros make sense as statements
because they die on failure.
Best Wishes
Phillip
And do away with needing to pass this through a function where we get
the size, and thus losing the type information before we can call
sizeof().
Then, as that series shows the implementation here could be pretty much
an exact line-for-line copy of what we have in cache.h, including the
same alloc_nr(), all without casts etc.
>
All of which seems much simpler than trying to maintain the constraint
that this must be usable in an expression.
Your commit message sort-of addresses this by mentioning that this
"returns −1 on allocation failure to accommodate other users of libxdiff
such as libgit2".
But since libgit2 won't use this, but only a copy of this xdiff code in
libgit2 I don't see how that makes sense. We're only talking about using
this in the xdiff code we maintain, and even if libgit2 wanted to use it
it could deal with not being able to use it in an expression, no?
1. https://lore.kernel.org/git/patch-5.7-3665576576f-20220708T140354Z-avarab@xxxxxxxxx/
