On Thu, Aug 13, 2020 at 03:32:56PM -0400, Eric Sunshine wrote:
> On Thu, Aug 13, 2020 at 11:54 AM Jeff King <peff@xxxxxxxx> wrote:
> > (As a side note, if we want to declare UNLEAK() a failure because nobody
> > cares enough to really use it, I'm OK with that, too).
>
> Perhaps the reason that UNLEAK() has not been particularly successful,
> in general, is that it requires extra knowledge and reasoning to know
> when to use it and how to do so properly. Couple that with the fact
> that the scope of cases where it can be used is quite narrow compared
> to sum total of all code in project for which we simply free resources
> when we're done with them. So, it's hard to keep the specialized
> UNLEAK() knowledge in one's head.
>
> Speaking from personal experience, the several times I have had to
> deal with UNLEAK(), I had to re-learn it from scratch each time. That
> meant studying the header comment, studying the implementation, and
> studying existing callers before things "clicked" enough to be able to
> feel confident about how to use it (assuming it wasn't false
> confidence).
I think this is really the meat of it. I never intended UNLEAK() to be
something people dealt with unless they were trying to get LSAN or
valgrind to run without complaining.
> That all represents a lot of cognitive overhead versus the common
> practice of simply freeing resources when you're done with them, which
> requires no extra cognitive load since it is something we think about
> _always_ when working with a language like C with no built-in garbage
> collection.
To be clear, I have no problem with _actually_ freeing resources if
that's an option. The point of UNLEAK() was:
- to help with structs that don't have an easy way to free all
elements (e.g., rev_info)
- to preempt arguments about whether calling free(buf) right before
programming exit is wasted effort. Whereas UNLEAK() is true
zero-cost for non-leak-checking builds.
- to avoid asking people to rewrite:
return foo(bar);
into:
ret = foo(bar);
free(bar);
return ret;
So we could go that direction, but I'd wait on it until somebody feels
like sinking some time into getting us leak-checker-clean.
In the meantime, I have a slight preference to leave UNLEAK() there as a
potential tool for somebody digging into leak-checkers. But we almost
certainly shouldn't be asking new authors to use it in reviews, etc.
TBH, I'm not sure why people starting sprinkling UNLEAK() around in the
first place. ;)
-Peff
