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