Re: Issue 2787: Sanitize usage of -DDEBUG, -DNDEBUG and assert (issue 236190043 by address@hidden)

[dak]

On 2015/05/16 18:18:49, Keith wrote:
> Probably a good idea, if assert() is really used like this.

> > assert is essentially stating that the program cannot
> > usefully continue if the assertion is not met

> There are uses of assert() where I suspect the output on conditions
that would
> violate the assertion, would be better than no output at all.

That's more a case for our "programming error" then.  Those are always
produced whereas assertions are completely removed With -DNDEBUG: it's
not like they just continue instead of aborting but still produce a
diagnostic: they are just silent then.

> Given that erroneous LilyPond is unlikely to do damage, as it does not
control
> medical devices or modify databases, erroneous output is probably more
useful
> than no output.

Assertions are not about "erroneous output".  They are about conditions
a program cannot usefully deal with and that should not occur.  In that
case, aborting with a diagnostic at the point of error is much easier to
debug than continuing and crashing on a followup problem.

> I think you have dealt with most of the inappropriate uses of
assert().

I don't think I actually touched any of them in this patch.  Several
have been changed to programming errors in the course of development
history in the last few years, so the quota should be better than when
this issue has been created.

> There
> are a few in 'page-breaking.cc' and 'constrained-breaking.cc' that
might cause
> trouble, and some floating point comparisons in 'simple-spacer.cc'
that might
> cause trouble on a cross-compile.

> We could throw lots of messy *.ly files at a debug build and see if
these break,
> or decide we've done that enough and let users do it for us in a
development
> release, to find any remaining inappropriate assert()s.

git grep assert lily

turns out several assertions with numerical conditions.  Which seems
sort of weird.  But we likely want to hear when those trigger.

https://codereview.appspot.com/236190043/