Re: [PATCH 0/2] UNLEAK style fixes

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Aug 14, 2020 at 6:35 AM Jeff King <peff@peff.net> wrote:
> On Thu, Aug 13, 2020 at 03:32:56PM -0400, Eric Sunshine wrote:
> > 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.
>
> 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.

I don't think it works that way in practice, though. There are enough
UNLEAK()'s sprinkled around that anyone working on or around code with
an existing UNLEAK() is compelled to understand/[re-]study it in order
to avoid breaking existing uses and/or to correctly mirror existing
uses when dealing with new resource allocations.

The same applies to patches. As a reviewer, I have two choices when I
see UNLEAK(): either I ignore it because I don't have the specialized
knowledge in my head (which makes me feel like my review is
ineffective), or I re-acquire the knowledge. And it's not just patches
like the ones in this series which are actively adjusting UNLEAK()
callers, but any patch which adds or removes an UNLEAK() corresponding
to the central meaty changes of the patch, or even a patch in which
UNLEAK() appears only in context lines, or even patches which don't
contains any UNLEAK() calls, but the source file to which the patch
applies does use UNLEAK(), if the reviewer consults the original
source code in addition to the patch.

> TBH, I'm not sure why people starting sprinkling UNLEAK() around in the
> first place. ;)

For the same reason that people are concerned about calling free() or
otherwise releasing or unlocking resources which they have acquired:
they're trying to be responsible. When a programmer sees UNLEAK()
being used in or around the code being changed, he or she will attempt
to maintain the fidelity of the existing code by being careful to
mimic existing nearby resource handling practices.