Re: [PATCH] mailinfo.c: move side-effects outside of assert

From: Kyle J. McKay <mackyle@gmail.com>
Date: Dec 19, 2016, 20:38 UTC
Message-ID: <A916CED6-C49D-41D8-A7EE-A5FEDA641F4A@gmail.com>
In-Reply-To: <20161219200259.nqqyvk6c72bcoaui@sigill.intra.peff.net>

On Dec 19, 2016, at 12:03, Jeff King wrote:

Show 33 quoted lines

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

I wondered exactly the same thing myself. I was hoping Jonathan would pipe in here with some analysis about whether this is:

   a) a super paranoid, just-in-case, can't really ever fail because  
by the time we get to this code we've already effectively validated  
everything that could cause check_header to return false in this case

-or-

   b) Yeah, it could fail in the real world and it should "die" (and  
probably have a test added that triggers such death)

-or-

   c) Actually, if check_header does return false we can keep going  
without problem

-or-

   d) Actually, if check_header does return false we can keep going by  
making a minor change that should be in the patch

I assume that since Jonathan added the code he will just know the answer as to which one it is and I won't have to rely on the results of my imaginary analysis. ;)

On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:

> ACK. I noticed this problem (and fixed it independently as a part of a
> huge patch series I did not get around to submit yet) while trying  
> to get
> Git to build correctly with Visual C.

Does this mean that Dscho and I are the only ones who add -DNDEBUG for release builds? Or are we just the only ones who actually run the test suite on such builds?

--Kyle