From: Felipe Contreras Date: Mon, 03 Jun 2013 16:10:05 GMT Subject: Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges Message-ID: In-Reply-To: <51ACBD68.4070506@lsrfire.ath.cx> On Mon, Jun 3, 2013 at 10:59 AM, René Scharfe wrote: > Am 03.06.2013 02:04, schrieb Felipe Contreras: >> >> On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe >> 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