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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 20, 2017, 01:45 UTC
Message-ID: <xmqqzi9qp2hq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170919210300.cbrdjqor6xuwd7bs@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 19 quoted lines

> On Tue, Sep 19, 2017 at 01:45:52PM -0700, Jonathan Tan wrote:
>
>> The following comments are assuming that we're going to standardize on
>> UNLEAK(var); (with the semicolon).
>
> Yeah, I assumed we would. We don't have to, since this really is sort-of
> magical, but I think the code looks better with it.
>
>> On Fri, 8 Sep 2017 02:38:41 -0400
>> Jeff King <peff@peff.net> wrote:
>> 
>> > +#ifdef SUPPRESS_ANNOTATED_LEAKS
>> > +extern void unleak_memory(const void *ptr, size_t len);
>> > +#define UNLEAK(var) unleak_memory(&(var), sizeof(var));
>> 
>> I would feel better if the semicolon was omitted. I don't think it
>> matters in this particular case, though.
>
> You end up with a double semi-colon. Some linters might complain.

Yeah, and it makes

	if (some condition)
		UNLEAK(var);
	else
		do_something_else_to(var);

a syntax error.

Should have spotted during the review; sorry.

Show 10 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.

Show 5 quoted lines

>
> I think both are worth doing, but the patch is already in next so we
> need to do it on top. Do you want to prepare a patch?
>
> -Peff