Consider checking for more warnings when compiling Widelands

Bug #1033615 reported by Hans Joachim Desserud
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
widelands
Fix Released
Medium
Hans Joachim Desserud

Bug Description

Recently, I have been looking into the extra warnings added by the compile option, and which are possible. Mainly to shake Widelands around a bit and see if any bugs fall out. And lo and behold, they did: bug 1033213 and bug 1033216.

 Keep in mind that the goal is to enable more warnings to catch more bugs, not to drown developers in noise. Every warning which is listed should be fixable, and have an obvious benefit when doing so. Ideally the code should become warning-free at some point, and adding the most noisy options will not help in this regard. Rather, by keeping the list short and the warnings listed relevant it is more likely that developers will notice when new ones are introduced and fix them when discovered.

First of all, I noticed we specify -Wno-attributes to ignore unexpected use of '__attribute__'. Currently adding this or leaving it out has no effect, can it be dropped? Also, the only instance I have seen of '__attribute__' was the piece of code which was removed to fix bug 744595.

I then experimented with adding the following warnings to WL_EXTRAWARNINGS:
-Wconversion, -Wformat, -Wformat-nonliteral, -Wformat-security, -Wformat-y2k, -Winit-self, -Winline, -Winvalid-pch, -Wlogical-op, -Wlong-long, -Wmissing-include-dirs, -Woverlength-strings, -Wpacked, -Wpadded, -Wpointer-arith, -Wredundant-decls, -Wshadow, -Wsign-conversion, -Wswitch-default, -Wswitch-enum, -Wsync-nand, -Wtrampolines, -Wundef, -Wunused, -Wunused-macros

Results and thoughts:
-Wconversion and -Wsign-conversion creates a lot of noise.
-Wlogical-op, -Wsync-nand and -Wtrampolines exists in GCC, but not clang 3.1 which will then fail to add all extra parameters. The latter two do not report any issues, but the first has already found bug 1033213. Therefore it would be nice to included that at least. Could be possible to add that to GCC only somehow?
-Winline only exists in GCC as well, not Clang. It is based on heuristics, so might be triggered by false positives. Was not triggered in any place in current trunk though.
-Wswitch-default finds some cases in GCC, but seem to be mostly false positives. (I will attach these below, and it would be nice if someone could double-check though.) Clang does not list anything.
-Wswitch-enum creates a lot of noise in GCC since it will list all the SDL_* constants when they are not used. Clang will group these and return a count instead of individual lines. It finds some cases which are not switching on SDL_*, but nothing that seems inherently wrong. Could someone take a look to double-check though?
-Wlong-long and -Wpadded is rather noisy.
-Wshadow adds some lines, but I am not sure how many of them are non-issues.
Note that the -Wformat* options require that -Wformat is set as well. This should be set by -Wall, but I had some problems with it. Might be because -Wall is not defined in the extra part, but rather the default one which is then merged together. It was trivial to add -Wformat explicitly to solve it though.
-Wunused should enable most -Wunused*, except -Wunused-macros?
-Wunused-macros finds some in minizip/unzip.cc (twice for some reason?), but also various others.
-Wformat-nonliteral finds a couple of issues in graphic/animation.cc.

 As a rule of thumb, I would like to add all warnings which found bugs in current trunk, i.e. -Wundef and -Wlogical-op. As commented above, the latter is only available on GCC though, so that needs to be resolved somehow.

Apart from that, the options which don't add any noise are good candidates. Fair enough, they don't find any issues or provide any benefits now, but as they don't generate noise they aren't harmful either. They might even catch issues in the future. Though, do see the note below in options and different compilers.

The options were tested with gcc 4.7.1 and clang 3.1. I have not looked at how recently these warnings were introduced and how horribly they will break stuff in older versions where they are not present. There is also the issue with if/how these options will work on other compilers, like Intel and Visual Studio. I know Clang aims to cover more or less the same options as GCC does, but I would be surprised if this is the case for others. As mentioned above, I have already noticed some options which are not available in Clang, and we would need some way of determining which options can be commonly used. As we at least currently support Visual Studio and don't want to exclude the idea of branching out to new compilers in the future this needs to be dealt with somehow.

The annoying part is that even if there is only one of the options which the compiler doesn't recognize, it will simply skip adding any of WL_EXTRAWARNINGS and continue without. Thus the final output will suggest less warnings that what is really there. When testing this, I used clang++ to compile Hello World feeding it all the options, in this case it would print warnings for the options it didn't know. When building WL though, these extra warnings would cause the addition of extra warnings to fail, and I could see that the compiler options set were lacking them.

PS. This is by no means an exhaustive list, but some of the warnings listed in the GCC manpage not covered by -Wall or -Wextra.

Related branches

Revision history for this message
Hans Joachim Desserud (hjd) wrote :

Could someone please check whether these are simply false positives, or if these switches should add a default statement?

Revision history for this message
Hans Joachim Desserud (hjd) wrote :

Adding all the 200+ SDL_* enums just sounds silly and doesn't add any real value. As such these warnings are noisy. Though could someone please look at whether they are relevant for the other cases?

Revision history for this message
Hans Joachim Desserud (hjd) wrote :

As mentioned above, this complains about minizip source files, so if we are not going to patch those, then these warnings will forever be listed making it impossible to become warning-free. If that's the case, I don't think it should be enabled, but it would be nice if someone could look into the other cases and see whether they should be used somewhere, should be removed or what to do with them.

Revision history for this message
Hans Joachim Desserud (hjd) wrote :

Some instance shadow previous local variables or global definitions which sounds like things we might not want to do. I am not sure if the majority which shadows variables in "this" is serious issues though.

Revision history for this message
Mark Scott (mxsscott) wrote :

What is the recommended GCC version for compiling Widelands with?

MacOSX 10.5.8 has 4.0 and 4.2 as installed choices, though I can whip up a version of anything including the development version 4.8-20121230 via MacPorts.

GCC 4.6+ supports "#pragma GCC diagnostic xxx" in the middle of functions, critical for silencing a warning coming from an external #define such as GLEW_VERSION_1_4 (which does an old-style cast).

Revision history for this message
Hans Joachim Desserud (hjd) wrote :

I am not sure what you are looking for in a recommended version, could you elaborate a bit? Ideally it should compile cleanly in all versions, as distros and users may have a myriad of different versions. If you look at bug 825957 you can see new posts after major new releases though. This is mainly based on the belief a newer version will a) find more/different warnings, b) search more thoroughly for existing warnings and c) hopefully report less false positives if any. So in order to find issues in the code I think it makes sense to use latest version of the compiler, though at the same time we don't want to cut us off so that users on older platforms will not be able to compile it because they have an older compiler.

Btw, from what I can see, we have a couple of instances of #pragma already.

Revision history for this message
SirVer (sirver) wrote :

There is no recommended version. Widelands can be compiled with VC C++ (do not know which version though), most gcc's (4.2 and higher for sure) and clang. We currently do not use C++11 do stay as compatible with older systems as possible - though I think we will start using some of the features soonsih.

Revision history for this message
Hans Joachim Desserud (hjd) wrote :

I've been thinking a bit about warnings which trigger a lot of noise (like the 200+ SDL_* enums). I think that now that we have the macro for silencing warnings, we can ignore these and focus on the real issues. I would still be wary about silencing warnings in case we shut up something we do want to hear about, but it might allow us to add some of these without getting the noise of false positives at the same time.

Revision history for this message
SirVer (sirver) wrote :

An extreme and ugly way would be to just redefine the macros in a way that do not trigger our warnings - e.g. by including the warning toggle switches into the definition. We could also include wrapper includes around the SDL headers that disable the warnings only for the SDL file.

As to this bug report, I believe -Wshadow should be enabled now by default, maybe also -Wswitch-enum from clang - but then we would need to silence it wherever we switch on SDL keycodes in the code base.

Changed in widelands:
assignee: nobody → Hans Joachim Desserud (hjd)
summary: - Consider checking for more warnings when compiling Widelands with
- WL_EXTRAWARNINGS
+ Consider checking for more warnings when compiling Widelands
Revision history for this message
Hans Joachim Desserud (hjd) wrote :

We've now added a lot of additional warnings which should allow us to notice and fix issues sooner when they are introduced.

Some options, like -Wconversion has not been added, but from my testing I remember some options would add an additional 100 000 lines! I don't expect anyone would sit down and go through that, so I've left it out for now.

Also, I've noticed that the -Wno-attributes which I originally removed silenced a lot of noise in older versions of Clang. I see no trace of it in 3.2, but it adds a lot more in 3.0. Should I perhaps reintroduce this and leave it in for another year or so, allowing everyone to upgrade. Considering nothing is printed in 3.2 I would assume these are simply false positives.

Changed in widelands:
status: New → In Progress
Revision history for this message
Nicolai Hähnle (nha) wrote :

Suggested warnings added in trunk in bzr6523.

Changed in widelands:
status: In Progress → Fix Committed
milestone: none → build18-rc1
Revision history for this message
Hans Joachim Desserud (hjd) wrote :

Thanks for merging the latest patch Nicolai. :)

I'm happy with the additional warnings which have been added now, and believe they will be helpful in catching and addressing issues early. Also a great thank you and high fives all around to the people which have been fixing the current warnings and improving code quality.

I'd just like to comment that the list of warnings is by no means set in stone, and it might change in the future. If the compilers add useful warnings we may want to include those, or we may want to disable some we currently have if they generate a lot of noise.

Revision history for this message
SirVer (sirver) wrote :

Released in build-18 rc1.

Changed in widelands:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.