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

From: Kyle J. McKay <mackyle@gmail.com>
Date: Dec 21, 2016, 05:54 UTC
Message-ID: <222ACFD4-ED9A-4B94-8BDD-3C70648A684B@gmail.com>
In-Reply-To: <20161220164526.qnwnmr7cvyycmw6a@sigill.intra.peff.net>

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

Show 23 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

> 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