From: Kyle J. McKay Date: Mon, 19 Dec 2016 20:38:26 GMT Subject: Re: [PATCH] mailinfo.c: move side-effects outside of assert Message-ID: In-Reply-To: <20161219200259.nqqyvk6c72bcoaui@sigill.intra.peff.net> On Dec 19, 2016, at 12:03, Jeff King wrote: > 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 >> --- >> >> 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