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