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

From: Jeff King <peff@peff.net>
Date: Dec 20, 2016, 16:45 UTC
Message-ID: <20161220164526.qnwnmr7cvyycmw6a@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1612201511480.54750@virtualbox>

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

Show 12 quoted lines

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