Fix 1477: Add (ly:expect-warning msg args) to suppress expected warnings

If a file contains (ly:expect-warning ....), the corresponding warning
string will be added to a list of expected warnings. If the corresponding
warning (or error) is triggered, it will not be printed to stderr, but the
string will be removed from the list. So, each ly:expect-warning, suppresses
exactly one occurrence of the warning. To suppress a warning multiple times,
call ly:expect-warning multiple times.
    
After one file is processed, the list of expected warnings is checked.
If it is not empty, it means that an expected warning was not triggered,
which might be a bug. So we print out a warning message about this fact.
    
This allows the regtests to check proper warning messages, without
polluting the console output with those warning messages.
All warnings that are actually printed in the regtests are a bug.
    
For translated error message, there are two approaches: If the warning
is created from scheme, simply use
       (ly:expect-warning (_ "msg with ~a") "args")
If the message is triggered from C++, the translated string is in printf
syntax, so we need to translate that and then convert it into a format
string for Scheme. This can be done with the new function
ly:translate-cpp-warning-scheme instead of _:
       (ly:expect-warning (ly:translate-cpp-warning-scheme "msg with %s") "args")
    
This patch also adapts all regtests that are supposed to print warnings
to this new approach. After this patch is applied, the only warnings/errors
that are printed out by a regtest run are warnings/errors that should
NOT be there (with the exception of warn-expected-warning-missing.ly).

[pkx166h, 2011-09-24 07:02:47]

Passes make and reg tests look ok but I guess someone who really knows should
check

See:

http://lilypond-stuff.1065243.n5.nabble.com/http-code-google-com-p-lilypond-i...

This has a zipped dir of the make check results.

[Graham Percival (old account), 2011-09-27 04:32:35]

Looks mostly good.  I would have preferred to have the code changes in a
separate patch from the regtest .ly editing, but that's not a major issue.

http://codereview.appspot.com/5037046/diff/15001/input/regression/stem-tremol...
File input/regression/stem-tremolo.ly (right):

http://codereview.appspot.com/5037046/diff/15001/input/regression/stem-tremol...
input/regression/stem-tremolo.ly:4: #(ly:expect-warning (_ "tremolo duration is
too long"))
Why do we expect those warnings here?  It sounds like somebody just screwed up
writing the regtest, so we should fix that instead of ignoring the warnings.

http://codereview.appspot.com/5037046/diff/15001/input/regression/tie-arpeggi...
File input/regression/tie-arpeggio.ly (right):

http://codereview.appspot.com/5037046/diff/15001/input/regression/tie-arpeggi...
input/regression/tie-arpeggio.ly:11: have to follow the lef-tied note directly.
When @code{tieWaitForNote}
left-tied

(not your patch, but it would be nice to fix it anyway)

[MikeSol, 2011-09-28 07:07:12]

LGTM

It'd be great to see the regtests run with -dwarning-as-error now that you've
done all this work.  Is that possible to incorporate into this patch?

Cheers,
MS

[reinhold_kainhofer.com, 2011-09-28 09:45:30]

Am Wednesday, 28. September 2011, 09:07:12 schrieben Sie:
> LGTM
> 
> It'd be great to see the regtests run with -dwarning-as-error now that
> you've done all this work.  Is that possible to incorporate into this
> patch?

Actually, I don't think that's a good idea, for several reasons:

1) The warning-as-error is currently only implemented in the Input class, not 
for warnings that are triggered by directly calling (ly:warning...). So we 
won't catch all warnings anyway.

2) Setting -dwarning-as-error means that any warning (e.g. a failed 
barcheck...) will cause a complete failure of the whole regtest build. I think 
this is way too volatile to be useful. A build should never fail, except for 
real problems.
Sometimes, the problems that appear are not so easily and quickly fixed, so 
that we might have periods of a week where a regtest prints out an undesired 
warning. Do we really want to hold up all patch testing simply because a 
warning surfaced in the regtests? 
I think that's out of proportion.

Cheers,
Reinhold

-- 
------------------------------------------------------------------
Reinhold Kainhofer, reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

[mike_apollinemike.com, 2011-09-28 09:54:49]

On Sep 28, 2011, at 11:45 AM, Reinhold Kainhofer wrote:

> Am Wednesday, 28. September 2011, 09:07:12 schrieben Sie:
>> LGTM
>> 
>> It'd be great to see the regtests run with -dwarning-as-error now that
>> you've done all this work.  Is that possible to incorporate into this
>> patch?
> 
> Actually, I don't think that's a good idea, for several reasons:
> 
> 1) The warning-as-error is currently only implemented in the Input class, not 
> for warnings that are triggered by directly calling (ly:warning...). So we 
> won't catch all warnings anyway.
> 

Is there a way to make it such that they are capture?.

> 2) Setting -dwarning-as-error means that any warning (e.g. a failed 
> barcheck...) will cause a complete failure of the whole regtest build. I think

> this is way too volatile to be useful. A build should never fail, except for 
> real problems.

I agree, but perhaps warnings like bar, bar number, and octave checks can also
have a throw/catch mechanism like the one you're implementing here.

> Sometimes, the problems that appear are not so easily and quickly fixed, so 
> that we might have periods of a week where a regtest prints out an undesired 
> warning. Do we really want to hold up all patch testing simply because a 
> warning surfaced in the regtests? 
> I think that's out of proportion.
> 

True, but the whole reason these problems show up in the master branch in the
first place is because people (including me) accidentally push stuff that
generates warnings.  If the regtest build failed because of -dwarning-as-error,
then the warning-generating code would not be pushed (assuming the person is not
a malevolent pusher and/or lazy).  That's the big advantage I see of using
-dwarning-as-error for regtests.

Cheers,
MS

[reinhold_kainhofer.com, 2011-09-28 10:35:58]

Am Wednesday, 28. September 2011, 11:54:38 schrieb mike@apollinemike.com:
> On Sep 28, 2011, at 11:45 AM, Reinhold Kainhofer wrote:
> > Am Wednesday, 28. September 2011, 09:07:12 schrieben Sie:
> >> LGTM
> >> 
> >> It'd be great to see the regtests run with -dwarning-as-error now that
> >> you've done all this work.  Is that possible to incorporate into this
> >> patch?
> > 
> > Actually, I don't think that's a good idea, for several reasons:
> > 
> > 1) The warning-as-error is currently only implemented in the Input class,
> > not for warnings that are triggered by directly calling (ly:warning...).
> > So we won't catch all warnings anyway.
> 
> Is there a way to make it such that they are capture?.

I'm working on it.

> > 2) Setting -dwarning-as-error means that any warning (e.g. a failed
> > barcheck...) will cause a complete failure of the whole regtest build. I
> > think this is way too volatile to be useful. A build should never fail,
> > except for real problems.
> 
> I agree, but perhaps warnings like bar, bar number, and octave checks can
> also have a throw/catch mechanism like the one you're implementing here.

Sure, if the warning is supposed to be there, it will be suppressed and the 
file won't fail. But I'm talking about those little oversights that happen 
when doing last-second changes before committing.

I don't think that an inadvertent warning should hold up all other developers 
from testing their own patches.

> True, but the whole reason these problems show up in the master branch in
> the first place is because people (including me) accidentally push stuff
> that generates warnings.  If the regtest build failed because of
> -dwarning-as-error, then the warning-generating code would not be pushed
> (assuming the person is not a malevolent pusher and/or lazy).  That's the
> big advantage I see of using -dwarning-as-error for regtests.

Well, I forgot my last reason:

3) We currently have several warnings in the regtest that should not be there. 
To see this, simply run "lilypond -l BASIC  *.ly" in input/regression.

With this patch applied, the only displayed warnings should be those from 
safe.ly and warn-expected-warning-missing.ly.

However, there are many more. Those are not expected, so suppressing them is 
like painting over the rust without actually fixing it. Unless all those are 
fixed, we simply cannot enable warning-as-error.

Attached is the current list of unexpected warning messages, coming from 23 
different regtests... Some of those are already reported as bugs, about others 
I'm not sure (i.e. I don't know what is really expected, in particular with 
the vertical spacing).

Cheers,
Reinhold

-- 
------------------------------------------------------------------
Reinhold Kainhofer, reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

[reinhold_kainhofer.com, 2011-09-28 10:46:17]

Am Wednesday, 28. September 2011, 12:35:55 schrieb Reinhold Kainhofer:
> Am Wednesday, 28. September 2011, 11:54:38 schrieb mike@apollinemike.com:
> > On Sep 28, 2011, at 11:45 AM, Reinhold Kainhofer wrote:
> > > 1) The warning-as-error is currently only implemented in the Input
> > > class, not for warnings that are triggered by directly calling
> > > (ly:warning...). So we won't catch all warnings anyway.
> > 
> > Is there a way to make it such that they are capture?.
> 
> I'm working on it.

Here's a patch that makes warning-as-error finally apply to all warnings:
http://codereview.appspot.com/5144050

Please review,
Reinhold

-- 
------------------------------------------------------------------
Reinhold Kainhofer, reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

[Graham Percival, 2011-09-28 10:52:43]

On Wed, Sep 28, 2011 at 12:35:55PM +0200, Reinhold Kainhofer wrote:
> Am Wednesday, 28. September 2011, 11:54:38 schrieb mike@apollinemike.com:
> > On Sep 28, 2011, at 11:45 AM, Reinhold Kainhofer wrote:
> > > 2) Setting -dwarning-as-error means that any warning (e.g. a failed
> > > barcheck...) will cause a complete failure of the whole regtest build. I
> > > think this is way too volatile to be useful. A build should never fail,
> > > except for real problems.
> 
> Sure, if the warning is supposed to be there, it will be suppressed and the 
> file won't fail. But I'm talking about those little oversights that happen 
> when doing last-second changes before committing.

People shouldn't be doing last-second changes before committing.
And if they do so anyway, they should push to the dev/staging
branch, so it will be checked before merging to master.

> I don't think that an inadvertent warning should hold up all other developers 
> from testing their own patches.

Of course not!  But that wouldn't happen, because the broken patch
won't get onto master because it'll die in the dev/staging branch.
:)

> Well, I forgot my last reason:
> 
> 3) We currently have several warnings in the regtest that should not be there.

This is a very good reason.

> However, there are many more. Those are not expected, so suppressing them is 
> like painting over the rust without actually fixing it. Unless all those are 
> fixed, we simply cannot enable warning-as-error.

Absolutely!

Cheers,
- Graham

[mike_apollinemike.com, 2011-09-28 11:03:33]

On Sep 28, 2011, at 12:35 PM, Reinhold Kainhofer wrote:

> Am Wednesday, 28. September 2011, 11:54:38 schrieb mike@apollinemike.com:
>> On Sep 28, 2011, at 11:45 AM, Reinhold Kainhofer wrote:
>>> Am Wednesday, 28. September 2011, 09:07:12 schrieben Sie:
>>>> LGTM
>>>> 
>>>> It'd be great to see the regtests run with -dwarning-as-error now that
>>>> you've done all this work.  Is that possible to incorporate into this
>>>> patch?
>>> 
>>> Actually, I don't think that's a good idea, for several reasons:
>>> 
>>> 1) The warning-as-error is currently only implemented in the Input class,
>>> not for warnings that are triggered by directly calling (ly:warning...).
>>> So we won't catch all warnings anyway.
>> 
>> Is there a way to make it such that they are capture?.
> 
> I'm working on it.
> 

Sehr gut.

>>> 2) Setting -dwarning-as-error means that any warning (e.g. a failed
>>> barcheck...) will cause a complete failure of the whole regtest build. I
>>> think this is way too volatile to be useful. A build should never fail,
>>> except for real problems.
>> 
>> I agree, but perhaps warnings like bar, bar number, and octave checks can
>> also have a throw/catch mechanism like the one you're implementing here.
> 
> Sure, if the warning is supposed to be there, it will be suppressed and the 
> file won't fail. But I'm talking about those little oversights that happen 
> when doing last-second changes before committing.
> 
> I don't think that an inadvertent warning should hold up all other developers 
> from testing their own patches.
> 

I feel that the category of "little oversights that happen when doing
last-second changes before committing" applies to other mistakes (forgotten
semi-colons, syntax errors, etc.).  All of these will crash build, all of these
are inadvertent, and all of these will stop development.  In general, the
argument "Law X, which rights wrong Y, will turn Y into criminality, which will
be an inconvenient for the taxpayers" seems to be OK in the short term but
potentially problematic in the long term :)

>> True, but the whole reason these problems show up in the master branch in
>> the first place is because people (including me) accidentally push stuff
>> that generates warnings.  If the regtest build failed because of
>> -dwarning-as-error, then the warning-generating code would not be pushed
>> (assuming the person is not a malevolent pusher and/or lazy).  That's the
>> big advantage I see of using -dwarning-as-error for regtests.
> 
> Well, I forgot my last reason:
> 
> 3) We currently have several warnings in the regtest that should not be there.

> To see this, simply run "lilypond -l BASIC  *.ly" in input/regression.
> 

I wouldn't mind scrubbing these.

> With this patch applied, the only displayed warnings should be those from 
> safe.ly and warn-expected-warning-missing.ly.
> 
> However, there are many more. Those are not expected, so suppressing them is 
> like painting over the rust without actually fixing it. Unless all those are 
> fixed, we simply cannot enable warning-as-error.
> 

I see what you mean.  A couple things:

1)  If a regtest exists to show painted rust (several do), this is not a
problem.
2)  You're right that this'll force us to get on bugs that we've been negligent
about, but at the same time, to merit the status of bug-free that we claim the
regtests portend, this should be the goal.  Granted, I think that these things
should be fixed before applying -dwarning-as-error, but it seems like a good
target.
3)  There could be a type of warning suppression called "open-question" (or
whatever) where we're not sure why the warning exists, or if there's anyway to
overcome it, or whatever.  This can be wrapped about these warnings and written
to the log file or console.  This way, we'll have a record of the rust we've
painted over.  The only danger then is, of course, if someone as an expedient
hides a warning that she shouldn't hide, but I think this behavior will be
generally frowned upon and not engaged in.

Cheers,
MS

[Reinhold, 2011-09-29 10:01:26]

Pushed as commits 
1ecdd56060e34a00b2be6b38029b286a601ea6f8 (code changes)
b8e19438b2d69674efcf408586950e384d5e4b06  (regtest changes)