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
