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

From: Kyle J. McKay <mackyle@gmail.com>
Date: Dec 22, 2016, 03:53 UTC
Message-ID: <99C4A905-D66B-4609-9E55-06F9BC301C74@gmail.com>
In-Reply-To: <F5001DF2-20C2-4757-997F-9D40BD48E1D9@gmail.com>

On Dec 21, 2016, at 18:21, Kyle J. McKay wrote:

Show 24 quoted lines

> 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 when you assume[1],  
>>> right? ;)
>>
>> 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 clearly specified in POSIX [1].
>
> If NDEBUG is defined then "assert(...)" disappears (and in a nice  
> way so as not to precipitate "unused variable" warnings).  "N" being  
> "No" or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning  
> Not DEBUG.  So the code that goes away when NDEBUG is defined is  
> clearly debug code.

I think there is a useful distinction here that I make that's worth sharing. Perhaps it's splitting hairs, but I categorize this "extra" code that we've been discussing ("assert(...)" or "if (!...) die(...)" or "verify(...)" into two groups:

1) DEBUG code

This is code that developers use when creating new features. Or helpful code that's needed when stepping through a program with the debugger to debug a problem. Or even code that's only used by some kind of external "test". It may be expensive, it may do things that should never be done in a build for wider consumption (such as write information to special log files, write special syslog messages etc.). Often this code is used in combination with a "-g" debug symbols build and possibly even a "-O0" or "-O1" option.

Code like this has no place in a release executable meant for general use by an end user.

2) DIAGNOSTIC code

This is near zero overhead code that is intended to be left in a release build meant for general use and normally sits there not doing anything and NOT leaching any performance out of the build either. Its sole purpose in life is to provide a trail of "bread crumbs" if the executable goes ***BOOM***. These "bread crumbs" should be just enough when combined with feedback from the unfortunate user who experienced the meltdown to re-create the issue in a real DEBUG build and find and fix the problem.

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.

What I'm saying is if they are diagnostic and not debug (and I'm not arguing one way or the other, but you've already indicated they are near zero overhead which suggests they are indeed diagnostic in nature), then they do not belong inside an "assert" which can be disabled with "NDEBUG". I'm arguing that "assert" is not intended for diagnostic code, but only debug code as used by nedmalloc. Having Git treat "NDEBUG" one way -- "no, no, do NOT define NDEBUG because that disables Git diagnostics and I promise you there's no performance penalty" -- versus nedmalloc -- "yes, yes please DO define NDEBUG unless you really need our slow debugging code to be present for debugging purposes" -- just creates needless unnecessary confusion.

--Kyle