On Mon, Jun 3, 2013 at 10:59 AM, René Scharfe <rene.scharfe@xxxxxxxxxxxxxx> wrote: > Am 03.06.2013 02:04, schrieb Felipe Contreras: >> >> On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe >> <rene.scharfe@xxxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html