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

On Mon, Jun 3, 2013 at 10:59 AM, René Scharfe
[off-list ref] wrote:

Am 03.06.2013 02:04, schrieb Felipe Contreras:

quoted

On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe
[off-list ref] wrote:

quoted

Am 03.06.2013 01:23, schrieb Felipe Contreras:

quoted

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.

quoted

There's no reason not to.

Only the minor ones already mentioned: More text,

Five characters.

quoted

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.

quoted

quoted

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