On Dec 20, 2016, at 08:45, Jeff King wrote:
On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:
On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
ACK. I noticed this problem (and fixed it independently as a part
of a
huge patch series I did not get around to submit yet) while
trying to
get Git to build correctly with Visual C.
Does this mean that Dscho and I are the only ones who add -DNDEBUG
for
release builds? Or are we just the only ones who actually run the
test
suite on such builds?
It seems you and I are for the moment the only ones bothering with
running
the test suite on release builds.
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? ;)
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.
Certainly I never have when deploying to GitHub's cluster (let alone
my
personal use), and I note that the Debian package also does not.
Yeah, I don't do it for my personal use because those are often not
based on a release tag so I want to see any assertion failures that
might happen and they're also not performance critical either.
So from my perspective it is not so much "do not bother with release
builds" as "are release builds even a thing for git?"
They should be if you're deploying Git in a performance critical
environment.
One of the
reasons I suggested switching the assert() to a die("BUG") is that the
latter cannot be disabled. We generally seem to prefer those to
assert()
in our code-base (though there is certainly a mix). If the assertions
are not expensive to compute, I think it is better to keep them in for
all builds. I'd much rather get a report from a user that says "I hit
this BUG" than "git segfaulted and I have no idea where" (of course I
prefer a backtrace even more, but that's not always an option).
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.
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, so
that makes sense.
So there's no way to get a non-release build of nedmalloc inside Git
then without hacking the Makefile? What if you need those assertions
enabled? Maybe NDEBUG shouldn't be defined by default for any files.
--Kyle
[1] https://www.youtube.com/watch?v=KEP1acj29-Y