Re: [PATCH 5/7] xdiff: use GALLOC_GROW(), not XDL_ALLOC_GROW()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 11, 2022, 10:48 UTC
Message-ID: <220711.861qur9ays.gmgdl@evledraar.gmail.com>
In-Reply-To: <c2bc5c79-e71b-f9dc-ba97-261d3454e150@gmail.com>

On Mon, Jul 11 2022, Phillip Wood wrote:

Show 16 quoted lines

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

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

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?

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

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