Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Jun 3, 2013, 15:59 UTC
Message-ID: <51ACBD68.4070506@lsrfire.ath.cx>
In-Reply-To: <CAMP44s2HotyK7hh0nFjCe+BBtu1pyqmkn6=yt9WV6UyhxjrXoQ@mail.gmail.com>

Am 03.06.2013 02:04, schrieb Felipe Contreras:

Show 23 quoted lines

> On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> Am 03.06.2013 01:23, schrieb Felipe Contreras:
>>
>>> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I
>>> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of
>>> 'if (ce != o->df_conflict_entry)'.
>>
>>
>> I did assume you meant the latter.
>>
>>
>>> There's no reason not to.
>>
>>
>> Only the minor ones already mentioned: More text,
>
> Five characters.
>
>> one more branch in object
>> code,
>
> Which might actually be more optimal.

The difference in absolute numbers will most certainly be within the noise for this one case.

>> no benefit except for some hypothetical future case that's caught by
>> the test suite anyway -- or by code review.
>
> That's not the benefit, the benefit is that the code is clearer.

I don't see that, and I don't like adding a check that I don't expect to be ever needed. Users are safe because the test suite is going to catch a missing check.

In general, I think those who introduce dependencies should add the necessary checks. They have to consider the invariants anyway, no matter how many checks you add beforehand for their convenience.

Show 6 quoted lines

>> I wonder if we already reached the point where we spent more time discussing
>> this change than the time needed by the envisioned developer to find and fix
>> the NULL check that suddenly became necessary. :)
>
> Maybe, but if what you want is to avoid the discussion, you could just
> add the extra five characters and be done with it.

Or you could submit a patch on top that adds the check. I'd send it out if you'd supply a commit message. My review comment would be "mostly harmless, but I don't like it very much because it's not needed now and probably won't ever".

But I'm more interested in a different way forward: Would it make sense to push the allocations (and frees) into the merge functions? Currently they duplicate one of the cache entries. Would the merge functions become too ugly or too big if they'd have to build them themselves, avoiding duplication? Would it be significantly faster?

René