[FFmpeg-devel] How to handle compiler warnings

[Diego Biurrun]

On Mon, Feb 04, 2008 at 01:12:11AM +0100, Michael Niedermayer wrote:
> On Mon, Feb 04, 2008 at 12:50:31AM +0100, Diego Biurrun wrote:
> > On Mon, Feb 04, 2008 at 12:18:27AM +0100, Michael Niedermayer wrote:
> > > On Sun, Feb 03, 2008 at 10:58:04PM +0100, Diego Biurrun wrote:
> > > > On Thu, Jan 31, 2008 at 03:28:11PM +0100, Michael Niedermayer wrote:
> > > > > On Thu, Jan 31, 2008 at 10:19:56AM +0100, Diego Biurrun wrote:
> > > > > > On Tue, Jan 29, 2008 at 11:45:15AM +0100, Diego Biurrun wrote:
> > > > > > > The topic has come up again, it's time to discuss the subject.  I
> > > > > > > propose to try to avoid compiler warnings as much as possible in order
> > > > > > > to
> > > > > > > 
> > > > > > > - have cleaner code,
> > > > > > > - have important warnings not be drowned out,
> > > > > > > - make FFmpeg a programming textbook.
> > > > > > > 
> > > > > > > This does not include warning fixes that slow things down or obfuscate
> > > > > > > the code, but if in doubt I personally would err on the side of fixing
> > > > > > > the warning.
> > > > > > 
> > > > > > OK, we pretty much seem to have consensus about this.  Should we add a
> > > > > > paragraph about warnings to the policy?
> > > > > 
> > > > > yes
> > > > 
> > > > Like this?
> > > 
> > > [...]
> > > >  @item
> > > > -    Do not change code to hide warnings without ensuring that the underlying
> > > > -    logic is correct and thus the warning was inappropriate.
> > > > +    Compiler warnings should be avoided unless the warning fix causes a
> > > > +    slowdown or obfuscates the code.
> > > >  @item
> > > 
> > > The sense behind warnings is to point to potential bugs or code with bad
> > > style. If a type of warning would always point to correct and clean code, that
> > > warning should be disabled not the code changed.
> > > Thus the remaining warnings could point to bugs or correct code. First one has
> > > to find out which of the 2 it is. If it is a bug, the bug should be fixed. If
> > > it is correct code, it should be changed so it does not generate a warning
> > > unless that causes a slowdown or obfuscates the code.
> > 
> > Is this better?
> 
> [...]
> >  @item
> > -    Do not change code to hide warnings without ensuring that the underlying
> > -    logic is correct and thus the warning was inappropriate.
> > +    Compiler warnings should be fixed if they point to real bugs. If not
> > +    the code should be changed so it does not generate warnings unless
> > +    this causes a slowdown or obfuscates the code.
> >  @item
> 
> IMHO not at all
> I dont understand why you remove that point from the policy.
> 
> We voted about adding a point which says remove warnings where it doesnt cause
> a slowdown or obfuscates the code. That is it would be a reason to reject a
> patch if it generated new easily avoidable warnings.
> 
> The problem i have with your formulation is that
> "Compiler warnings should be fixed if they point to real bugs"
> You here concentrate on the warning but thats totally wrong, if theres a
> bug, the bug should be fixed, the warning is not relevant. If after fixing
> the bug there still is a warning left, that is a seperate issue to deal with.

OK, I have committed a slightly reworded version of your suggestion.  If
anybody wants further changes it should be easier to work from there.

Diego