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

On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:

quoted

On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:

quoted

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

So from my perspective it is not so much "do not bother with release
builds" as "are release builds even a thing for git?".  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).

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.

-Peff