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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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.

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:

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


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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]