Re: configure.ac: add -Werror to default compiler options

[Rob Shearman]

2008/9/18 Austin English <austinenglish(a)gmail.com>:
> Should help avoid bugs like bug 15266 and promote more proper, portable code.
> Tried compiling Wine with -Werror, got a few interesting results. First one:
>
> austin(a)austin-desktop:~/wine-git/dlls/jscript$ make
> gcc -c -I. -I. -I../../include -I../../include  -D__WINESRC__  -D_REENTRANT
> -fPIC -Wall -Werror -pipe -fno-strict-aliasing -Wdeclaration-after-statement
> -Wwrite-strings -Wpointer-arith  -g -O2  -o engine.o engine.c
> cc1: warnings being treated as errors
> engine.c: In function 'var_statement_eval':
> engine.c:500: warning: 'hres' is used uninitialized in this function

Or it could just be that they had a different compiler version to you
and so the warning didn't appear for them. This is the trouble with
using -Werror in an uncontrolled environment - a developer using one
version of the compiler could commit code that compiles cleanly for
them, but not for another developer using a different compiler version
and so stop them from being able to build Wine. That's fine if we want
to do it, but we have to consider whether it is worth the hassle for
whatever increase in quality we get from it.

-- 
Rob Shearman

configure.ac: add -Werror to default compiler options

[Francois Gouget]

On Thu, 18 Sep 2008, Rob Shearman wrote:
[...]
> > engine.c:500: warning: 'hres' is used uninitialized in this function
>
> Or it could just be that they had a different compiler version to you
> and so the warning didn't appear for them. This is the trouble with
> using -Werror in an uncontrolled environment

Maybe patchwatcher could detect new warnings (e.g. using git-blame as I 
described before) and either flag the patch outright as incorrect, or 
give it an intermediate state between bad patches (those that don't 
compile or cause test regressions) and good ones (that compile and don't 
cause test regressions).

In either case it would be good to run the conformance tests on such 
patches anyway so the submitter can fix both the compilation warning and 
any test issue in one roundtrip.

-- 
Francois Gouget <fgouget(a)free.fr>              http://fgouget.free.fr/
        A polar bear is a cartesian bear after a coordinate transform.

configure.ac: add -Werror to default compiler options

[Juan Lang]

> Maybe patchwatcher could detect new warnings (e.g. using git-blame as I
> described before) and either flag the patch outright as incorrect, or
> give it an intermediate state between bad patches (those that don't
> compile or cause test regressions) and good ones (that compile and don't
> cause test regressions).

Since Rob was noncommittal about whether -Werror was a good or a bad
idea, I'll take a stance:  I think it's a bad one.  For one thing,
different optimization levels produce different warnings.  For
another, some warnings are just plain wrong.  In some cases the "fix"
to quiet the warning is uglier than the warning.

Having a code base that has hundreds of warnings is also a problem,
but we're fortunate enough not to be in that situation.

I think our current system isn't bad:  if one of us makes a mistake
and it gets past Alexandre, someone often points out the problem and
it gets fixed.  With -Werror, a mistake (or a gcc bug) becomes
everyone else's problem.  I don't think that's a step in the right
direction.
--Juan

configure.ac: add -Werror to default compiler options

[Maarten Lankhorst]

Hi All,

Rob Shearman schreef:
> 2008/9/18 Austin English <austinenglish(a)gmail.com>:
> > Should help avoid bugs like bug 15266 and promote more proper, portable code.
> > Tried compiling Wine with -Werror, got a few interesting results. First one:
> >
> > austin(a)austin-desktop:~/wine-git/dlls/jscript$ make
> > gcc -c -I. -I. -I../../include -I../../include  -D__WINESRC__  -D_REENTRANT
> > -fPIC -Wall -Werror -pipe -fno-strict-aliasing -Wdeclaration-after-statement
> > -Wwrite-strings -Wpointer-arith  -g -O2  -o engine.o engine.c
> > cc1: warnings being treated as errors
> > engine.c: In function 'var_statement_eval':
> > engine.c:500: warning: 'hres' is used uninitialized in this function
>
> Or it could just be that they had a different compiler version to you
> and so the warning didn't appear for them. This is the trouble with
> using -Werror in an uncontrolled environment - a developer using one
> version of the compiler could commit code that compiles cleanly for
> them, but not for another developer using a different compiler version
> and so stop them from being able to build Wine. That's fine if we want
> to do it, but we have to consider whether it is worth the hassle for
> whatever increase in quality we get from it.

-Wall -Werror should not be in configure, some quick test found the 
following issues:

* xinerama, mesa, randr and alsa are no longer found.
* Various functions like snprintf, strcasecmp, strdup, strncasecmp, 
vsnprintf, memmove, are no longer found.
* EXTRACFLAGS no longer has -fno-strength-reduce

Until those things are addressed, I don't believe it's a good idea to 
enable those flags.

This was found by diffing config.status created with ./configure 
CFLAGS='-g -O2' and CFLAGS='-g -O2 -Wall -Werror'
config.status is created by configure, and it will create all other 
files like makefiles and config.h

If you want to build with -Wall -Werror, do "make CFLAGS='-g -O2 -Wall 
-Werror'". With -O0 some warnings that -Wall warns for are not shown 
because they require optimization.

The mesa "up-to-date" check seems to have been in since before the year 
2000, and as such I don't know if it's still required. I don't mind it 
being taken out.

Cheers,
Maarten.