[MPlayer-dev-eng] [PATCH] More warnings

[Uoti Urpala]

On Mon, 2011-02-07 at 07:54 +0100, Reimar Döffinger wrote:
> On Mon, Feb 07, 2011 at 12:22:45AM +0200, Uoti Urpala wrote:
> > On Sun, 2011-02-06 at 22:17 +0100, Reimar Döffinger wrote:
> > > The few cases where they don't are easy enough to ignore.
> > 
> > This is not true either. One large and general class of counterexamples
> > is code that passes values forward without itself modifying them (strchr
> > in the standard C library is a canonical example).
> 
> And most people do not write a C library.

And strict "const" use is still bad practice for them. Those problems
are in no way limited to "libraries".

> And strchr could have avoided the issue by returning the offset instead
> of a pointer (though at a performance cost, so it is not always a good idea).

It's possible to add workarounds for const problems. It's also possible
to write the whole program using "if () goto xyz;" as the only control
structure. Neither is very likely to improve overall program quality
though.

> Also the lack of usefulness you claim is not convincing, e.g. in FFmpeg
> quite a few codecs implementing the API incorrectly (modifying the input
> buffer) slipped by because of not using const or casting it away "accidentally".

Yes, "accidentally". In quotes. Do you really imagine you can force the
compiler to give warnings if someone has misunderstood the requirements
or otherwise _tries_ to implement undesirable semantics? That has little
to do with reality. (Btw I think "quite a few" is an exaggeration.)

> And on several programs I used -Wwrite-strings the first time it ended up
> showing stuff like string constant pointers being freed (yes, obviously
> valgrind never ran over that code either).

I'm not sure what you're trying to say here. The pointer actually
pointed to a string constant? And that ran on some system without
crashing? Or if the code never ran in practice, why the reference to
valgrind?

Anyway, that some warning once indicated a bug in some program does not
mean it would be a good idea to enable it. It's always a tradeoff
between useful output and false alerts. And for these options it's not
just false alerts; you need to modify otherwise perfectly fine code to
make them usable at all. The overall effect is negative: using the same
effort in other ways would achieve significantly more - or the
workarounds can actually make the end result strictly worse, even
without factoring in the increased effort.