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

[Felipe Contreras <felipe.contreras@gmail.com>]

On Mon, Jun 3, 2013 at 10:59 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 03.06.2013 02:04, schrieb Felipe Contreras:
>>
>> 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.

If you want to ignore the performance, you should ignore the branch as well.

>>> 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.

It's called self-documenting code; by adding a check for the NULL
pointer, we are stating that ce can be NULL, if we don't do that,
people reading that code would need to figure that out themselves.

> Or you could submit a patch on top that adds the check.

I already sent a patch that has that check.

http://article.gmane.org/gmane.comp.version-control.git/225972

-- 
Felipe Contreras