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

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

On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:

quoted

quoted

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.

quoted

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:

quoted

On Dec 20, 2016, at 08:45, Jeff King wrote:

quoted

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

quoted

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