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