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

[Phillip Wood <phillip.wood123@gmail.com>]

On 13/07/2022 11:48, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jul 13 2022, Phillip Wood wrote:
>>
>> 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.
> 
> Well, there's a lot to understand :) So it's also an implicit comment on
> the complexity of your series.

I'm surprised you think it is complex, the implementation of 
XDL_ALLOC_GROW() and its helper is pretty short.

> In case it wasn't clear the main thrust of what I've been commenting on
> here is asking why what you have here needs to *structurally* look the
> way it does, i.e. why you think the malloc() & free() naming can't
> resolve to libgit2's wrappers (per the thread ending at [1]).

I think the different structural approach stems in part from the subtle 
differences between ALLOC_GROW() and XDL_ALLOC_GROW(). Once one 
appreciates that the latter needs to free the original allocation on 
overflow and allocation failure and work with code that uses long rather 
than size_t then there is not much code left to be shared between them.

> And, if we can't end up with an xdiff/* in our tree that doesn't have
> return value checking flying in the face of xmalloc's NORETURN()
> behavior on allocation failures.

I don't think xmalloc() is marked as NORETURN in wrapper.h so the 
compiler would need somehow determine that from looking at the 
implementation in wrapper.c but even if it is using LTO I'm not sure 
it'll have that information when creating xdiff/lib.a

> But yes, the suggested replacement isn't behaving exactly as yours does,
> but I think that's just an implementation detail as far as the stuctural
> questions above go. I.e.:
> 
>   * You're trying to fix a long-standing overflow issue in alloc_nr(),
>     but in such a way that only leaves xdiff/* with the fix.

I didn't set out to fix that issue per-se, I only realized it existed 
when I reviewed this series.

>     Can't we address that seperately (e.g. turning alloc_nr into a static
>     inline function using the st_* helper), and then make xdiff and
>     cache.h use that new shared helper?

As I've said before xdiff does not want to die on overflow so it cannot 
use st_mult(). Also you cannot assume that you're dealing with size_t 
when you do the overflow check in alloc_nr() so I think it is a tricky 
problem to solve.

>   * You seem to be set on the idea that you absolutely must be rewriting
>     large parts of the existing allocation macro, because you'd really
>     like to use it as an expression v.s. a statement.

It is not just that - there are plenty of differences that stem from 
returning an error rather that dying that reduce the amount of common 
code. Having a separate definition was also driven by a desire to keep 
the xdiff code self contained.

This will likely be the last message from me in this thread for a while 
- I'm going offline later next week and want to make sure I have time to 
review Stolee's rebase patches before then.

Best Wishes

Phillip