Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives

["Martin Ågren" <martin.agren@gmail.com>]

On 5 September 2017 at 15:05, Jeff King <peff@peff.net> wrote:
>   1. It can be compiled conditionally. There's no need in
>      normal runs to do this free(), and it just wastes time.
>      By using a macro, we can get the benefit for leak-check
>      builds with zero cost for normal builds (this patch
>      uses a compile-time check, though we could clearly also
>      make it a run-time check at very low cost).
>
>      Of course one could also hide free() behind a macro, so
>      this is really just arguing for having UNLEAK(), not
>      for its particular implementation.

Like Stefan, I didn't quite follow 1. until after I had read the points
below it. But it's still a very good commit message (as always).

> diff --git a/builtin/commit.c b/builtin/commit.c
> index b3b04f5dd3..de775d906c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1819,5 +1819,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>                 print_summary(prefix, &oid, !current_head);
>
>         strbuf_release(&err);
> +       UNLEAK(sb);
>         return 0;
>  }

These are both strbufs, so this ends up being a bit inconsistent. What
would be the ideal end state for these two and all other such
structures? My guess is "always UNLEAK", as opposed to carefully judging
whether foo_release() would/could add any significant overhead.

In other words, it would be ok/wanted with changes such as "let's UNLEAK
bar, because ..., and while at it, convert the existing foo_release to
UNLEAK for consistency" (or per policy, for smaller binary, whatever).
Or "if it ain't broken, don't fix it"? Did you think about this, or was
it more a random choice?

Martin