On 10/2/2018 6:44 PM, Stefan Beller wrote:
My preference is to avoid them in the name of simplicity. If you're
using "make SANITIZE=leak test" to check for leaks, it will skip these
cases. If you're using valgrind, I think these may be reported as
"reachable". But that number already isn't useful for finding real
leaks, because it includes cases like the "foo" above as well as
program-lifetime globals.
The only argument (IMHO) for such an UNLEAK() is that it annotates the
location for when somebody later changes the function to "return -1"
instead of dying. But if we are going to do such annotation, we may as
well actually call free(), which is what the "return" version will
ultimately have to do.
Heh, that was part of my reasoning why we'd want to have *something*.
I'd actually be _more_ favorable to calling free() instead of UNLEAK()
there, but I'm still mildly negative, just because it may go stale (and
our leak-checking wouldn't usefully notice these cases). Anybody
converting that die() to a return needs to re-analyze the function for
what might need to be released (and that includes non-memory bits like
descriptors, too).
Sounds reasonable, so then the consensus (between Martin, you and me)
is to drop the UNLEAK.
Thanks for the discussion here. I'll drop the UNLEAK for now and think
about how to remove the die() calls from commit-graph.c in a later series.
Thanks,
-Stolee
