On 16/02/2022 14:38, Ævar Arnfjörð Bjarmason wrote:
[...]
More generally I think this patch is taking the wrong approach, why are
we trying to the members of a stack variable in xdl_do_diff(), when that
variable isn't ours, but is created by our caller? Why aren't they
dealing with it?
They are not dealing with it because they do not initialize it - it is
an "out" parameter that is used to return data to the caller. This
patch changes the logic to "whoever initializes it is responsible for
freeing it if there is an error". By doing that we localize the error
handling to xdl_do_diff() and can leave the callers unchanged.
Yes, I'm saying that we're needlessly piling on complexity by continuing
with this pattern in the xdiff/ codebase. I think it's fair to question
the direction in general.
I think if there were a lot of these bugs that needed fixing then the
wholesale changes to the way memory is managed you seem to be suggesting
would be worth it. However I think the existing code is mostly correct
so such a change would be a lot of churn and would absorb a lot of
reviewer time that would be better spent elsewhere compared to fixing up
the code we have.
Best Wishes
Phillip
