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

2016-12-22 Thread Junio C Hamano
Jeff King writes: > Well, no, I mostly just said that I do not think there is any point in > defining NDEBUG in the first place, as there is little or no benefit to > removing those asserts from the built product. > ... > Sure, if you want to mass-convert them, doing so with a

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

2016-12-21 Thread Jeff King
On Wed, Dec 21, 2016 at 07:53:15PM -0800, Kyle J. McKay wrote: > It seems to me what you are saying is that Git's "assert" calls are > DIAGNOSTIC and therefore belong in a release build -- well, except for the > nedmalloc "assert" calls which do not. Yes, I think that is a good way of thinking

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

2016-12-21 Thread Kyle J. McKay
On Dec 21, 2016, at 18:21, Kyle J. McKay wrote: On Dec 21, 2016, at 07:55, Jeff King wrote: On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote: I wasn't aware anybody actually built with NDEBUG at all. You'd have to explicitly ask for it via CFLAGS, so I assume most people

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

2016-12-21 Thread Jeff King
On Wed, Dec 21, 2016 at 06:21:37PM -0800, Kyle J. McKay wrote: > > Kind of. If it's a configuration that nobody[1] in the Git development > > community intended to support or test, then isn't the person triggering > > it the one making assumptions? > > No, I don't think so. NDEBUG is very

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

2016-12-21 Thread Kyle J. McKay
On Dec 21, 2016, at 07:55, Jeff King wrote: On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote: I wasn't aware anybody actually built with NDEBUG at all. You'd have to explicitly ask for it via CFLAGS, so I assume most people don't. Not a good assumption. You know what happens

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

2016-12-21 Thread Jeff King
On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote: > > I wasn't aware anybody actually built with NDEBUG at all. You'd have to > > explicitly ask for it via CFLAGS, so I assume most people don't. > > Not a good assumption. You know what happens when you assume[1], right? ;) Kind

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

2016-12-20 Thread Kyle J. McKay
On Dec 20, 2016, at 08:45, Jeff King wrote: On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote: 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

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

2016-12-20 Thread Jeff King
On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote: > > 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

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

2016-12-20 Thread Johannes Schindelin
Hi Kyle, On Mon, 19 Dec 2016, Kyle J. McKay wrote: > 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

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

2016-12-19 Thread Junio C Hamano
Jonathan Tan writes: >>> This is obviously an improvement, but it makes me wonder if we should be >>> doing: >>> >>> if (!check_header(mi, >inbody_header_accum, mi->s_hdr_data)) >>> die("BUG: some explanation of why this can never happen"); >>> >>> which perhaps

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

2016-12-19 Thread Jonathan Tan
On 12/19/2016 12:38 PM, Kyle J. McKay wrote: 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

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

2016-12-19 Thread Kyle J. McKay
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(...))

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

2016-12-19 Thread Jeff King
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,

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

2016-12-19 Thread Johannes Schindelin
Hi, On Sat, 17 Dec 2016, 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

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

2016-12-17 Thread Kyle J. McKay
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