From: Jeff King <peff@peff.net>
Date: Mon, 19 Dec 2016 20:03:00 GMT
Subject: Re: [PATCH] mailinfo.c: move side-effects outside of assert
Message-ID: <20161219200259.nqqyvk6c72bcoaui@sigill.intra.peff.net>
In-Reply-To: <900a55073f78a9f19daca67e468d334@3c843fe6ba8f3c586a21345a2783aa0>

On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:

> Since 6b4b013f18 (mailinfo: handle in-body header continuations,
> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an
> assert of the form:
> 
> 	assert(call_a_function(...))
> 
> The function in question, check_header, has side effects.  This
> means that when NDEBUG is defined during a release build the
> function call is omitted entirely, the side effects do not
> take place and tests (fortunately) start failing.
> 
> Move the function call outside of the assert and assert on
> the result of the function call instead so that the code
> still works properly in a release build and passes the tests.
> 
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> ---
> 
> Notes:
>     Please include this PATCH in 2.11.x maint

This is obviously an improvement, but it makes me wonder if we should be
doing:

  if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
	die("BUG: some explanation of why this can never happen");

which perhaps documents the intended assumptions more clearly. A comment
regarding the side effects might also be helpful.

-Peff