Issue 2787: Sanitize usage of -DDEBUG, -DNDEBUG and assert

Issue 2787: Sanitize usage of -DDEBUG, -DNDEBUG and assert

The compiler option -DNDEBUG is no longer being used: -DNDEBUG disables
the assert function, and assert is essentially stating that the program
cannot useful continue if the assertion is not met.  -DNDEBUG is
basically an option for compiling an application to a limited amount of
ROM when aborting with a diagnostic is not preferable to crashing.

This is not the case for LilyPond.  So expensive debugging options now
are enabled with -DDEBUG instead.  At the current point of time, setting
this is still tied to the configure option --disable-optimising.  It
might make sense to move this to a separate option --enable-testing or
similar, but that would require simultaneous changes to the Patchy
testing framework.

[dak, 2015-05-12 17:44:41]

Add --enable-checking option, let it be implied by --disable-optimising for now

[Keith, 2015-05-16 18:18:49]

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.

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.

I think you have dealt with most of the inappropriate uses of assert().  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.

[dak, 2015-05-16 19:01:50]

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.