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

From: Kyle J. McKay <mackyle@gmail.com>
Date: Dec 22, 2016, 02:21 UTC
Message-ID: <F5001DF2-20C2-4757-997F-9D40BD48E1D9@gmail.com>
In-Reply-To: <20161221155539.aykcmkuzqvq733ri@sigill.intra.peff.net>

On Dec 21, 2016, at 07:55, Jeff King wrote:

Show 13 quoted lines

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

Considering the wide deployment and use of Git at this point I think rather the opposite to be true that "Git does Not require DEBUGging code to be enabled for everyday use." The alternative that it does suggests it's not ready for prime time and quite clearly that's not the case.

Show 8 quoted lines

>> I've been defining NDEBUG whenever I make a release build for quite  
>> some
>> time (not just for Git) in order to squeeze every last possible  
>> drop of
>> performance out of it.
>
> I think here you are getting into superstition. Is there any single
> assert() in Git that will actually have an impact on performance?

You have suggested there is and that Git is enabling NDEBUG for exactly that reason -- to increase performance:

Show 6 quoted lines

>> On Dec 20, 2016, at 08:45, Jeff King wrote:
>>
>>> I do notice that we set NDEBUG for nedmalloc, though if I am  
>>> reading the
>>> Makefile right, it is just for compiling those files. It looks like
>>> there are a ton of asserts there that _are_ potentially expensive

Show 11 quoted lines

>> Perhaps Git should provide a "verify" macro.  Works like "assert"  
>> except
>> that it doesn't go away when NDEBUG is defined.  Being Git-provided  
>> it could
>> also use Git's die function.  Then Git could do a global replace of  
>> assert
>> with verify and institute a no-assert policy.
>
> What would be the advantage of that over `if(...) die("BUG: ...")`? It
> does not require you to write a reason in the die(), but I am not sure
> that is a good thing.

You have stated that you believe the current "assert" calls in Git (excluding nedmalloc) should not magically disappear when NDEBUG is defined. So precluding a more labor intensive approach where all currently existing "assert(...)" calls are replaced with an "if (!...) die(...)" combination, providing a "verify" macro is a quick way to make that happen. Consider this, was the value that Jonathan provided for the "die" string immediately obvious to you? It sure wasn't to me. That means that whoever does the "assert(...)" -> "if(!...)die" swap out may need to be intimately familiar with that particular piece of code or the result will be no better than using a "verify" macro.

I'm just trying to find a quick and easy way to accommodate your wishes without redefining the semantics of NDEBUG. ;)

--Kyle

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/assert.h.html