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); > > 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
