Re: configure.ac: add -Werror to default compiler options
On Wed, Aug 31, 2011 at 11:13 PM, Octavian Voicu wrote: > On Thu, Sep 1, 2011 at 4:27 AM, Dan Kegel wrote: >> At which point it would probably be a fine idea to add -Werror by default; >> buildbot will help keep everyone in sync, even if they're using a compiler >> that doesn't catch as many warnings as the one buildbot uses. > > Have you tried compiling on 64-bit? There are still a handful of > compile warnings there (most in oleaut32/tmarshal.c; then > setupapi/devinst.c; dsound+winmm). OK, finally tried this myself. It looks like oleaut32/tmarshal.c is the only serious obstacle left. - Dan
Re: configure.ac: add -Werror to default compiler options
1 sep 2011 kl. 03:27 skrev Dan Kegel: > At which point it would probably be a fine idea to add -Werror by default; > buildbot will help keep everyone in sync, even if they're using a compiler > that doesn't catch as many warnings as the one buildbot uses. We use -Werror at my company, but I think you should really think twice before enabling it for a distributed package. New compiler versions add new warnings. Usually correctly so, but there's no way to know what warnings will be produced by what compiler, and keeping up with new compilers requires a bit of effort. We make sure to always test new compilers separately before upgrading on the developing machines. I think most users will probably just see that the package fails to build, and assume it's broken. Ps. I know there's also a few warnings while building on OS X. I don't have access to it now, or I'd send the list. -- Pelle Johansson
Re: configure.ac: add -Werror to default compiler options
On Thu, Sep 1, 2011 at 8:52 AM, Dan Kegel wrote: > On Thu, Sep 1, 2011 at 1:35 AM, Jerome Leclanche wrote: >> Here is a warning report from a 64bit system. I get a lot of warnings >> on both 32 and 64bit wines. Many of them are "variable set but not >> used" (unused error returns, ...). >> >> http://bugs.winehq.org/show_bug.cgi?id=26768 was filed a while ago for >> the oleaut32 spam. > > Wow. Guess we have a ways to go. Finally dug up AJ's position on the question: http://www.winehq.org/pipermail/wine-devel/2008-September/069203.html So, no -Werror by default in git. Buildbot may at some point start complaining about patches that introduce new warnings, though. Dan
Re: configure.ac: add -Werror to default compiler options
Am 01.09.2011 17:52, schrieb Dan Kegel: > On Thu, Sep 1, 2011 at 1:35 AM, Jerome Leclanche wrote: >> Here is a warning report from a 64bit system. I get a lot of warnings >> on both 32 and 64bit wines. Many of them are "variable set but not >> used" (unused error returns, ...). >> >> http://bugs.winehq.org/show_bug.cgi?id=26768 was filed a while ago for >> the oleaut32 spam. > > Wow. Guess we have a ways to go. > - Dan > > Also beside Micheal's comment -Werror is a bad idea for crosscompiling, compiling with other compilers, compiling on new/uncommon systems, compiling on other architectures and so on... -- Best Regards, André Hentschel
Re: configure.ac: add -Werror to default compiler options
On Thu, Sep 1, 2011 at 1:35 AM, Jerome Leclanche wrote: > Here is a warning report from a 64bit system. I get a lot of warnings > on both 32 and 64bit wines. Many of them are "variable set but not > used" (unused error returns, ...). > > http://bugs.winehq.org/show_bug.cgi?id=26768 was filed a while ago for > the oleaut32 spam. Wow. Guess we have a ways to go. - Dan
Re: configure.ac: add -Werror to default compiler options
(resent with correct from-address, please ignore the previous one) 1 sep 2011 kl. 03:27 skrev Dan Kegel: > At which point it would probably be a fine idea to add -Werror by default; > buildbot will help keep everyone in sync, even if they're using a compiler > that doesn't catch as many warnings as the one buildbot uses. We use -Werror at my company, but I think you should really think twice before enabling it for a distributed package. New compiler versions add new warnings. Usually correctly so, but there's no way to know what warnings will be produced by what compiler, and keeping up with new compilers requires a bit of effort. We make sure to always test new compilers separately before upgrading on the developing machines. I think most users will probably just see that the package fails to build, and assume it's broken. Ps. I know there's also a few warnings while building on OS X. I don't have access to it now, or I'd send the list. -- Per Johansson
Re: configure.ac: add -Werror to default compiler options
Dan Kegel wrote: > (Reviving > http://www.winehq.org/pipermail/wine-devel/2008-September/069266.html > ) > > After > http://www.winehq.org/pipermail/wine-patches/2011-August/106150.html and > http://www.winehq.org/pipermail/wine-patches/2011-August/106151.html, > configuring with -Wall -Werror gives the right results, > judging by a quick diff of config.h and config.status. > > (sys/asoundlib.h is not detected, but that's ok, it's marked as obsolete, > and wine uses alsa/asoundlib.h instead.) > > This takes care of Maarten's concerns from the earlier thread. > > Then building yields only a tiny handful of errors: > > dllfunc.c: In function ‘AMovieDllRegisterServer2’: > dllfunc.c:180: warning: dereferencing type-punned pointer will break > strict-aliasing rules > qualitycontrol.c: In function ‘QualityControlImpl_Notify’: > qualitycontrol.c:70: warning: dereferencing type-punned pointer will > break strict-aliasing rules > transform.c: In function ‘TransformFilterImpl_QueryInterface’: > transform.c:248: warning: dereferencing type-punned pointer will break > strict-aliasing rules > ../../include/winternl.h: In function ‘get_vm86_teb_info’: > ../../include/winternl.h:2660: warning: dereferencing type-punned > pointer will break strict-aliasing rules > ... > register.c: In function ‘create_registrar’: > register.c:66: warning: dereferencing type-punned pointer will break > strict-aliasing rules > > Those are potential, if unlikely, bugs; we ought to either fix them or > add -fno-strict-aliasing > to CFLAGS for the seven or so affected .c files. > > At which point it would probably be a fine idea to add -Werror by default; > buildbot will help keep everyone in sync, even if they're using a compiler > that doesn't catch as many warnings as the one buildbot uses. You can try submitting a patch to add -Werror by default but I already know the patch status that it will get: REJECTED!!11!! You might get away with enabling it for the buildbot as you can enforce a controlled environment. But enabling it in general is such a bad idea that it isn't funny. No clue why people insist to still do it. Ask packagers what they think about upstreams that do that, disabling that is one of the first actions they do. bye michael
Re: configure.ac: add -Werror to default compiler options
On Thu, Sep 1, 2011 at 4:27 AM, Dan Kegel wrote: > At which point it would probably be a fine idea to add -Werror by default; > buildbot will help keep everyone in sync, even if they're using a compiler > that doesn't catch as many warnings as the one buildbot uses. Have you tried compiling on 64-bit? There are still a handful of compile warnings there (most in oleaut32/tmarshal.c; then setupapi/devinst.c; dsound+winmm). Octavian
re: configure.ac: add -Werror to default compiler options
(Reviving http://www.winehq.org/pipermail/wine-devel/2008-September/069266.html ) After http://www.winehq.org/pipermail/wine-patches/2011-August/106150.html and http://www.winehq.org/pipermail/wine-patches/2011-August/106151.html, configuring with -Wall -Werror gives the right results, judging by a quick diff of config.h and config.status. (sys/asoundlib.h is not detected, but that's ok, it's marked as obsolete, and wine uses alsa/asoundlib.h instead.) This takes care of Maarten's concerns from the earlier thread. Then building yields only a tiny handful of errors: dllfunc.c: In function ‘AMovieDllRegisterServer2’: dllfunc.c:180: warning: dereferencing type-punned pointer will break strict-aliasing rules qualitycontrol.c: In function ‘QualityControlImpl_Notify’: qualitycontrol.c:70: warning: dereferencing type-punned pointer will break strict-aliasing rules transform.c: In function ‘TransformFilterImpl_QueryInterface’: transform.c:248: warning: dereferencing type-punned pointer will break strict-aliasing rules ../../include/winternl.h: In function ‘get_vm86_teb_info’: ../../include/winternl.h:2660: warning: dereferencing type-punned pointer will break strict-aliasing rules ... register.c: In function ‘create_registrar’: register.c:66: warning: dereferencing type-punned pointer will break strict-aliasing rules Those are potential, if unlikely, bugs; we ought to either fix them or add -fno-strict-aliasing to CFLAGS for the seven or so affected .c files. At which point it would probably be a fine idea to add -Werror by default; buildbot will help keep everyone in sync, even if they're using a compiler that doesn't catch as many warnings as the one buildbot uses. - Dan
Re: configure.ac: add -Werror to default compiler options
Hi All, Rob Shearman schreef: > 2008/9/18 Austin English <[EMAIL PROTECTED]>: > >> 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: >> >> [EMAIL PROTECTED]:~/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.
Re: configure.ac: add -Werror to default compiler options
On Fri, 19 Sep 2008, Dan Kegel wrote: [...] > So my alternate suggestion is for patchwatcher to reject patches that > fail with -Werror. > (And work around the couple of bogus errors I listed above.) It does not even have to build with -Werror, just to grep the log for warnings, and warn if there are new ones. The benefit is that because the compilation succeeded, you still get to test the patch for other errors, notably by running the conformance tests. Then the patchwatcher report can complain about both warnings, and any conformance regressions that it identified. In fact, we could even have patchwatcher compile with multiple compilers, a bleeding one and an older one, gcc 2.95 say, so we catch these nameless unions earlier ;-) (but I don't mind continuing with my weekly gcc 2.95 build) -- Francois Gouget <[EMAIL PROTECTED]> http://fgouget.free.fr/ It really galls me that most of the computer power in the world is wasted on screen savers. Chris Caldwell from the GIMPS project http://www.mersenne.org/prime.htm
Re: configure.ac: add -Werror to default compiler options
OK, I just ran with -Wall -Werror, and got a grand total of four errors: freetype.c:1051: warning: 'name.string_len' is used uninitialized in this function Looks bogus. engine.c:2128: warning: 'str' may be used uninitialized in this function Bogus. context.c:80: warning: 'update_minfilter' may be used uninitialized in this function context.c:80: warning: 'update_magfilter' may be used uninitialized in this function Real! Looks like one slipped by yesterday: http://source.winehq.org/git/wine.git/?a=commitdiff;h=9533a5cbbf76cf4b9013ed90dd96e0f3995d44f3 That's not a bad haul. Now, if everyone were building with -O2 -Wall -Werror, that would have been caught sooner. But hey, if Alexandre doesn't like the idea, I guess it won't fly. So my alternate suggestion is for patchwatcher to reject patches that fail with -Werror. (And work around the couple of bogus errors I listed above.) That gets us the desired benefits without running into the problems Alexandre and Marcus pointed out. How's that sound? - Dan
Re: configure.ac: add -Werror to default compiler options
On Fri, Sep 19, 2008 at 04:35:45PM +0200, Alexandre Julliard wrote: > "Dan Kegel" <[EMAIL PROTECTED]> writes: > > > Agreed, to an extent. A user who is trying to compile with a really > > whacky toolchain (say, a C compiler on an Amiga, a mainframe, or a > > wristwatch) > > should expect some errors, and we should not try to avoid those if they > > reflect > > real problems that need to be solved before wine can run properly. > > You don't need a wacky toolchain. All you need is a slightly old gcc, or > a platform that not everybody has access to, like Mac OS. I periodically Or a newer gcc, or -O3, or -D_FORTIFY_SOURCE=2 (default in SUSE and Redhat) etc etc. If someone wants -Werror, he can always set CFLAGS for himself ;) Ciao, Marcus
Re: configure.ac: add -Werror to default compiler options
"Dan Kegel" <[EMAIL PROTECTED]> writes: > Agreed, to an extent. A user who is trying to compile with a really > whacky toolchain (say, a C compiler on an Amiga, a mainframe, or a wristwatch) > should expect some errors, and we should not try to avoid those if they > reflect > real problems that need to be solved before wine can run properly. You don't need a wacky toolchain. All you need is a slightly old gcc, or a platform that not everybody has access to, like Mac OS. I periodically build on Mac OS and check for warnings, and there are always a few that creep in, like size_t printf formats. Do you really think Wine would be better off if it failed to build on Mac 90% of the time? Is printing a size_t with %d really so dangerous that it needs to break the build? > And I also feel pretty strongly that compiler warnings have > some value, and we should pay attention to them. Right > now we're plugging our ears and going "la la la la la I can't hear you", > and that seems a bit careless. As Wine aims for higher and > higher quality levels, eventually we will have to change our ways here. That's absolutely not true. We are trying very hard to avoid warnings, I don't commit patches that add warnings, and many people are sending fixes when they find warnings on their platform. That doesn't mean they should break the build. > If we do it in the right way, it could end up increasing > our code quality quite a bit without inconveniencing any users. It wouldn't make any difference to code quality, since we are already warnings-free on most standard builds. > The right way is to slowly fix all the warnings -- like Andrew Talbot > is doing -- > and slowly encourage all wine developers to crank up their warning > levels to the max. Once we've voluntarily cleared out nearly all the > warnings, we can then have a flag day to clean out all the rest, > and switch -Werror on. That will keep errors from creeping back in. Everybody is free to build their tree with -Werror, but there's no way that it will become the default. -- Alexandre Julliard [EMAIL PROTECTED]
Re: configure.ac: add -Werror to default compiler options
On Fri, Sep 19, 2008 at 1:17 AM, Alexandre Julliard <[EMAIL PROTECTED]> wrote: > A compile failure is a serious bug, and we should do everything > possible to avoid them. Agreed, to an extent. A user who is trying to compile with a really whacky toolchain (say, a C compiler on an Amiga, a mainframe, or a wristwatch) should expect some errors, and we should not try to avoid those if they reflect real problems that need to be solved before wine can run properly. And I also feel pretty strongly that compiler warnings have some value, and we should pay attention to them. Right now we're plugging our ears and going "la la la la la I can't hear you", and that seems a bit careless. As Wine aims for higher and higher quality levels, eventually we will have to change our ways here. > Making -Werror the default is the worse thing we could do in that respect. If we do it in the right way, it could end up increasing our code quality quite a bit without inconveniencing any users. The right way is to slowly fix all the warnings -- like Andrew Talbot is doing -- and slowly encourage all wine developers to crank up their warning levels to the max. Once we've voluntarily cleared out nearly all the warnings, we can then have a flag day to clean out all the rest, and switch -Werror on. That will keep errors from creeping back in.
Re: configure.ac: add -Werror to default compiler options
"Dan Kegel" <[EMAIL PROTECTED]> writes: > On Thu, Sep 18, 2008 at 5:54 PM, Juan Lang <[EMAIL PROTECTED]> wrote: >> If a commit doesn't break on Alexandre's [and patchwatcher's] box, >> and it gets committed, but it does break on some other machine, what >> then? Who fixes it? > > I suppose the answer there is "patchwatcher builds with multiple compilers" > and/or "affected users with bum compilers turn off -Werror" > and/or "we disable the errors that have high failure rates on some compilers". No, the answer is "developers who really want their build to die on simple warnings can turn it on". We need Wine to build properly on a wide range of platforms and compiler versions, there's simply no way to ensure that the compile will always be warning free. It is very important that everybody be able to build Wine and give it a try, even non-developers. That's why I'm so strict about portable code, proper configure checks, and conservative makefiles. There's nothing worse than downloading a project you want to play with and find out you can't even compile it. A compile failure is a serious bug, and we should do everything possible to avoid them. Making -Werror the default is the worse thing we could do in that respect. -- Alexandre Julliard [EMAIL PROTECTED]
Re: configure.ac: add -Werror to default compiler options
On Thu, Sep 18, 2008 at 8:46 PM, Dan Kegel <[EMAIL PROTECTED]> wrote: > On Thu, Sep 18, 2008 at 5:17 PM, James Hawkins <[EMAIL PROTECTED]> wrote: >> I think the important thing for Dan's case is that he also used -Wall, >> which we should never turn on by default, especially not with -Werror. >> Correct me if I've made an incorrect assumption. > > Many serious projects always use both -Wall and -Werror, including Chrome. And not so serious ones. We actually try to use -Werror everywhere in ROS except the Wine code and a few other third party modules. I think it would be a good idea to enable it on a selective basis in Wine, start with programs,server,libs,etc and then slowly turn it on for dlls. -- Steven Edwards "There is one thing stronger than all the armies in the world, and that is an idea whose time has come." - Victor Hugo
Re: configure.ac: add -Werror to default compiler options
> I suppose the answer there is "patchwatcher builds with multiple compilers" > and/or "affected users with bum compilers turn off -Werror" > and/or "we disable the errors that have high failure rates on some compilers". Sure, that makes as much sense as what I suggested. Consider me mollified, but I withhold the right for an "I told you so" if this leads to silly bugs and patches flying around ;-) > You obviously don't understand my plans with patchwatcher and valgrind :-) Okay, now you're piquing my interest :) > What is "draconian" but a synonym for "dictatotorial"? Sounds like it > would fit in just fine with Wine's maintainer's style :-) You've got me there :) --Juan
Re: configure.ac: add -Werror to default compiler options
On Thu, Sep 18, 2008 at 5:54 PM, Juan Lang <[EMAIL PROTECTED]> wrote: > If a commit doesn't break on Alexandre's [and patchwatcher's] box, > and it gets committed, but it does break on some other machine, what > then? Who fixes it? I suppose the answer there is "patchwatcher builds with multiple compilers" and/or "affected users with bum compilers turn off -Werror" and/or "we disable the errors that have high failure rates on some compilers". > I think that individually, we can turn it on from time to time to see > what pops up, just like running with valgrind. You obviously don't understand my plans with patchwatcher and valgrind :-) > But forcing everyone to run with it seems draconian. What is "draconian" but a synonym for "dictatotorial"? Sounds like it would fit in just fine with Wine's maintainer's style :-) - Dan
Re: configure.ac: add -Werror to default compiler options
> You're pretty negative on this idea, aren't you? Let me whittle away at > it a bit before we reject it as impractical. Sure, have fun. I've used -Wall -Werror with success in the past with a medium-sized code base, and used it as a stick to keep people from checking in dodgy code. So it's not that it's always impractical. The issue I have is that a distributed development environment makes it hard, as does maintaining a portable code base linked against many possible versions of libraries. If a commit doesn't break on Alexandre's box, and it gets committed, but it does break on some other machine, what then? Who fixes it? I think that individually, we can turn it on from time to time to see what pops up, just like running with valgrind. But forcing everyone to run with it seems draconian. --Juan
Re: configure.ac: add -Werror to default compiler options
On Thu, Sep 18, 2008 at 5:17 PM, James Hawkins <[EMAIL PROTECTED]> wrote: > I think the important thing for Dan's case is that he also used -Wall, > which we should never turn on by default, especially not with -Werror. > Correct me if I've made an incorrect assumption. Many serious projects always use both -Wall and -Werror, including Chrome. It would be nice if Wine also became serious in this regard, but it will take some work to get there. - Dan
Re: configure.ac: add -Werror to default compiler options
On Thu, Sep 18, 2008 at 5:10 PM, Juan Lang <[EMAIL PROTECTED]> wrote: >> Are you sure about that? When I configure with -Wall -Werror, >> I can't even configure properly; one gets the errors >> >> configure: libxrandr development files not found, XRandr won't be supported. > > This is precisely part of the problem with -Werror. You've now caused > some configure checks to fail that, presumably, were passing before. > Are they worth making warning-free? Some of the warnings may be in > headers we don't control. What do we do then? You're pretty negative on this idea, aren't you? Let me whittle away at it a bit before we reject it as impractical. >> And if one blows past those, "make depend" fails. In fact, everything >> fails pretty spectacularly. > > So a broken config leads to a broken compile? No, there seem to be multiple severe breakages not caused by the slightly broken config. But let me gather some more data. - Dan
Re: configure.ac: add -Werror to default compiler options
Juan wrote: > 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. ... > some warnings are just plain wrong. In some cases the "fix" > to quiet the warning is uglier than the warning. In those cases, we can disable 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. Are you sure about that? When I configure with -Wall -Werror, I can't even configure properly; one gets the errors configure: libxrandr development files not found, XRandr won't be supported. configure: libxinerama development files not found, multi-monitor setups won't be supported. configure: WARNING: Old Mesa headers detected. Consider upgrading your Mesa libraries. OpenGL and Direct3D won't be supported. And if one blows past those, "make depend" fails. In fact, everything fails pretty spectacularly. We should probably have a look at -Wall errors in more detail and breadth before we assume that we're ok. - Dan
Re: configure.ac: add -Werror to default compiler options
On Thu, Sep 18, 2008 at 7:20 PM, Juan Lang <[EMAIL PROTECTED]> wrote: >> I think the important thing for Dan's case is that he also used -Wall, >> which we should never turn on by default, especially not with -Werror. >> Correct me if I've made an incorrect assumption. > > -Wall is turned by default: > EXTRACFLAGS="-Wall -pipe" > K, wasn't sure. Thanks for the info. -- James Hawkins
Re: configure.ac: add -Werror to default compiler options
> I think the important thing for Dan's case is that he also used -Wall, > which we should never turn on by default, especially not with -Werror. > Correct me if I've made an incorrect assumption. -Wall is turned by default: EXTRACFLAGS="-Wall -pipe" --Juan
Re: configure.ac: add -Werror to default compiler options
On Thu, Sep 18, 2008 at 7:10 PM, Juan Lang <[EMAIL PROTECTED]> wrote: >> Are you sure about that? When I configure with -Wall -Werror, >> I can't even configure properly; one gets the errors >> >> configure: libxrandr development files not found, XRandr won't be supported. >> configure: libxinerama development files not found, multi-monitor >> setups won't be supported. >> configure: WARNING: Old Mesa headers detected. Consider upgrading your >> Mesa libraries. >> OpenGL and Direct3D won't be supported. > > This is precisely part of the problem with -Werror. You've now caused > some configure checks to fail that, presumably, were passing before. > Are they worth making warning-free? Some of the warnings may be in > headers we don't control. What do we do then? > I think the important thing for Dan's case is that he also used -Wall, which we should never turn on by default, especially not with -Werror. Correct me if I've made an incorrect assumption. -- James Hawkins
Re: configure.ac: add -Werror to default compiler options
> Are you sure about that? When I configure with -Wall -Werror, > I can't even configure properly; one gets the errors > > configure: libxrandr development files not found, XRandr won't be supported. > configure: libxinerama development files not found, multi-monitor > setups won't be supported. > configure: WARNING: Old Mesa headers detected. Consider upgrading your > Mesa libraries. > OpenGL and Direct3D won't be supported. This is precisely part of the problem with -Werror. You've now caused some configure checks to fail that, presumably, were passing before. Are they worth making warning-free? Some of the warnings may be in headers we don't control. What do we do then? > And if one blows past those, "make depend" fails. In fact, everything > fails pretty spectacularly. So a broken config leads to a broken compile? I guess I'm not seeing the severity of that. --Juan
Re: configure.ac: add -Werror to default compiler options
> 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
Re: configure.ac: add -Werror to default compiler options
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 <[EMAIL PROTECTED]> http://fgouget.free.fr/ A polar bear is a cartesian bear after a coordinate transform.
Re: configure.ac: add -Werror to default compiler options
2008/9/18 Austin English <[EMAIL PROTECTED]>: > 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: > > [EMAIL PROTECTED]:~/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
