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

From: Jeff King <peff@peff.net>
Date: Sep 20, 2017, 02:28 UTC
Message-ID: <20170920022838.tlkb5qlmctlstqb6@sigill.intra.peff.net>
In-Reply-To: <xmqqzi9qp2hq.fsf@gitster.mtv.corp.google.com>

On Wed, Sep 20, 2017 at 10:45:05AM +0900, Junio C Hamano wrote:

Show 12 quoted lines

> >> > +#else
> >> > +#define UNLEAK(var)
> >> 
> >> I think this should be defined to be something (for example, "do {}
> >> while (0)"), at least so that we have compiler errors when UNLEAK(var)
> >> is used incorrectly (for example, without the semicolon) when
> >> SUPPRESS_ANNOTATED_LEAKS is not defined.
> >
> > Seems reasonable.
> 
> Hmph, I am not so sure about this one.  But I agree that the
> semicolon must go.

I thought we had run into some issues with a compiler or linter complaining about null statements before. But they _are_ pretty common, so maybe I'm mis-remembering (or maybe it was something like the if/else conditional you showed earlier).

At any rate, I think Jonathan's point is that writing:

  UNLEAK(foo)

will silently pass in a normal build, and only much later will somebody run a leak-checking build and see the compile error.

Of course people adding UNLEAK()s are likely to be compiling leak-check builds to confirm that they've silenced the warning, so maybe it's not that important a case to catch.

-Peff