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