On Wed, Dec 21, 2016 at 06:21:37PM -0800, Kyle J. McKay wrote:
> > Kind of. If it's a configuration that nobody[1] in the Git development
> > community intended to support or test, then isn't the person triggering
> > it the one making assumptions?
>
> No, I don't think so. NDEBUG is very clearly specified in POSIX [1].
I know what NDEBUG is, and I know it is widely used in other projects.
My claim is only that I do not think that the use of NDEBUG was
generally considered when people wrote assert() in git. That is
certainly true of me. I don't know about other developers.
> > I think here you are getting into superstition. Is there any single
> > assert() in Git that will actually have an impact on performance?
>
> You have suggested there is and that Git is enabling NDEBUG for exactly that
> reason -- to increase performance:
Sorry if I wasn't clear, but I meant in Git itself, not in the nedmalloc
compat code. I think it's worth considering them separately because the
latter is not part of most Git builds in the first place, and certainly
it is written in a different style, and may have different assumptions
about how people might build it.
I do not know the nedmalloc code well at all. I gave a brief read of the
results of "git grep 'assert('" and it looked like some of its
assertions are more involved than simple comparisons. So I guessed that
perhaps the reason that NDEBUG was added there when the code was
imported is that there is a performance difference there. But that's
just a guess. It may or may not be borne out by measurements.
I'd be surprised if any of the assertions in the rest of git have a
noticeable impact (and I could not detect one by running t/perf).
> > What would be the advantage of that over `if(...) die("BUG: ...")`? It
> > does not require you to write a reason in the die(), but I am not sure
> > that is a good thing.
>
> You have stated that you believe the current "assert" calls in Git
> (excluding nedmalloc) should not magically disappear when NDEBUG is defined.
Well, no, I mostly just said that I do not think there is any point in
defining NDEBUG in the first place, as there is little or no benefit to
removing those asserts from the built product.
> So precluding a more labor intensive approach where all currently existing
> "assert(...)" calls are replaced with an "if (!...) die(...)" combination,
> providing a "verify" macro is a quick way to make that happen. Consider
> this, was the value that Jonathan provided for the "die" string immediately
> obvious to you? It sure wasn't to me. That means that whoever does the
> "assert(...)" -> "if(!...)die" swap out may need to be intimately familiar
> with that particular piece of code or the result will be no better than
> using a "verify" macro.
Sure, if you want to mass-convert them, doing so with a macro similar to
assert is the simplest way. I don't think we are in a huge hurry to do
that conversion though. I'm not complaining that NDEBUG works as
advertised by disabling asserts. I'm just claiming that it's largely
pointless in our code base, and I'd consider die("BUG") to be our
"usual" style. If any change is to be made, it would be to suggest
people prefer die("BUG") as a style guideline for future patches. But I
haven't seen anybody agree (or disagree) with the notion, so I'd
hesitate to impose my style suggestion without more discussion in that
area.
If we _did_ accept that as a style guideline, then we could move over
existing assert() calls over time (either as janitorial projects, or as
people touch the related code). But there's not a pressing need to do it
quickly.
-Peff
