Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives
- Date: Wed, 6 Sep 2017 19:16:00 +0200
- From: Martin Ågren <martin.agren@xxxxxxxxx>
- Subject: Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives
On 5 September 2017 at 15:05, Jeff King <peff@xxxxxxxx> 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);
> + 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?