Hi Ævar
On 11/07/2022 11:48, Ævar Arnfjörð Bjarmason wrote:
On Mon, Jul 11 2022, Phillip Wood wrote:
Hi Ævar
On 08/07/2022 15:20, Ævar Arnfjörð Bjarmason wrote:
Replace the recently introduced XDL_ALLOC_GROW() with invocations of
the GALLOC_GROW() from git-shared-util.h.
As this change shows the macro + function indirection of
XDL_ALLOC_GROW() is something we needed only because the two callsites
we used it in wanted to use it as an expression, and we thus had to
pass the "sizeof" down.
Let's just check the value afterwards instead, which allows us to
use
the shared macro, we can also remove xdl_reallo(), this was its last
user.
I don't think this expression->statement change is an
improvement.
I think the use-as-statement is prettier too, but I think the uglyness
of having to pass down the sizeof() & re-implementing the macro version
of the alloc-or-die variant outweights that.
I think this is partly a choice between prioritizing ease of
implementation or ease of use for callers.
This change also removes the overflow checks that are
present in XDL_ALLOC_GROW()[...]
We end up calling st_mult(), which does that overflow check. Do you mean
that the POC shimmy layer I showed in another reply for libgit2 doesn't
have an st_mult() that detects overflows?
I was referring to
#define alloc_nr(x) (((x)+16)*3/2)
in cache.h. XDL_ALLOC_GROW() detects overflows when growing the number
of items as well as when calculating the number of bytes to allocate.
That's true, but as noted downthread of that we can & could ship that as
part of the shimmy layer, but that's unrelated to this change.
In your pre-image you use LONG_MAX instead of UINTMAX_MAX & I don't see
(but maybe I haven't looked at it carefully enough) how it does the same
dying on overflows. Doesn't it just fall back to LONG_MAX?
It does not die on overflow as we want to return errors rather than die
in the xdiff code. It uses long to match the existing code.
Part of this is that it's not clear to me from your commit(s) why you
need to rewrite alloc_nr() and rewrite (or drop?) st_mult().
So that we don't die on overflow and so that the xdiff code is self
contained.
I'm a bit disappointed that this patch seems to have been written
without really taking the time to understand exactly what the code it is
replacing is doing.
Best Wishes
Phillip
and fails to free the old allocation when
realloc() fails. It is not a like for like replacement.
Yes, we should have a free() there. Wel spotted. But again, doing that
as part of the "gently" branch seems preferrable to have duplicate
versions for expression (non-fatal) v.s. statement (fatal) variants.
