On Thu, Feb 11, 2021 at 10:40:45AM +0100, Johannes Schindelin wrote: > On Thu, 11 Feb 2021, Ævar Arnfjörð Bjarmason wrote: > > > On Wed, Feb 10 2021, Johannes Schindelin wrote: > > > > > On Fri, 5 Feb 2021, Ævar Arnfjörð Bjarmason wrote: > > > > > >> Add a diff_free() function to free anything we may have allocated in > > >> the "diff_options" struct, and the ability to make calling it a noop > > >> by setting "no_free" in "diff_options". > > > > > > Why do we need a `no_free` flag? Why not simply set the `free()`d (or > > > `fclose()`d) attributes to `NULL`? > > Hmm. That was not even clear to me until I read this reply. > > Doesn't this indicate that the closing is done at the wrong layer? If we > want to call a function N times and only at the last iteration should it > clean up our resources, doesn't that indicate that the clean-up should be > pulled out from that function? > > I thought that's exactly what you did, but I must have glanced over > something obvious... I think the issue then is that every caller must now be modified to do the clean up (which otherwise happens automatically when flushing the diff). So I definitely agree that the current API is weird and backwards. But flipping it now may introduce new leaks in existing callers. -Peff
