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

On Dec 20, 2016, at 08:45, Jeff King wrote:

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

quoted

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.

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