Consider checking for more warnings when compiling Widelands
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-
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/
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
- Widelands Developers: Pending requested
-
Diff: 52 lines (+19/-9)1 file modifiedCMakeLists.txt (+19/-9)
- Widelands Developers: Pending requested
-
Diff: 14 lines (+2/-2)1 file modifiedCMakeLists.txt (+2/-2)
- SirVer: Approve
- Nicolai Hähnle: Approve
-
Diff: 144 lines (+28/-26)6 files modifiedCMakeLists.txt (+1/-1)
src/editor/editorinteractive.cc (+4/-3)
src/minizip/unzip.cc (+3/-0)
src/scripting/pluto.cc (+2/-0)
src/ui_basic/messagebox.cc (+14/-14)
src/ui_fsmenu/options.cc (+4/-8)
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 |
Could someone please check whether these are simply false positives, or if these switches should add a default statement?