On Sep 28, 2011, at 12:35 PM, Reinhold Kainhofer wrote:
Am Wednesday, 28. September 2011, 11:54:38 schrieb
address@hidden:
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