Merge lp:~hjd/widelands/more-warnings into lp:widelands

[Hans Joachim Desserud]

Adding another batch of additional warnings. Mainly finds a couple of issues reported by -Wswitch-enum and -Wswitch-default, as well as a couple of others which doesn't seem to hurt.

As mentioned, this includes -Wswitch-enum which has been silenced in several places, since some places will trigger warnings on 200+ SDL_* enums not used. I hope this won't ignore any important warnings, though adding 200 different elements just to have them there doesn't seem ideal either.

Some notes:
The two methods touched in src/ui_fsmenu/options.cc look identical to me, but I haven't investigated thoroughly.
Had to add explicit brackets to a conditional in editorinteractive.cc, otherwise it would fail to compile claiming it found an else without an if.

[Nicolai Hähnle (nha)]

Thank you for doing this. I am a bit worried about the discussion in the linked bug reports that compilers ignore enabled warnings if the encounter some warning option that they don't know about. Isn't there some way to make this more robust, in case the supported compiler options change at some point? Of course, this may be something for a separate merge and doesn't necessarily need to be addressed here.

The explicit brackets are a good thing.

I oppose -Wswitch-enum. The appropriate way to indicate our intention in those switch(SDL_keysym) cases is by adding a default: label, and that should therefore be sufficient to stop the compiler from complaining. According to the gcc documentation, -Wswitch-enum complains even when a default: label exists, and we should therefore use -Wswitch rather than -Wswitch-enum. Note that, according to the documentation, -Wswitch is part of -Wall.

Other than that, I like your changes.

review: Needs Fixing

[SirVer (sirver)]

@nicolai: If you have a suggestion of warning detection besides compiling something with the warning enabled (which is quite painful indeed with cmake), let it here.

I agree that we should only get a switch warning when both conditions are true

a) there is no default: label
b) not all cases of an enum are handled in the switch

if this is given by -Wswitch, we should not use switch-enum. hjd, could you double check this please? Maybe also what clang does.

[Hans Joachim Desserud (hjd)]

Nicolai: You are of course right, thanks for pointing that out. I didn't know it would include missing enums even when a default case is present, which sounds a tad bit overzealous even to me. I checked both the g++ man page and a few sample warnings reported and can confirm this is indeed how the option behaved. (I wasn't able to find any place documenting Clang's warning options in detail, but every source agree they want to be compatible with GCC, so I would assume they have the same behaviour). I've now backed it out again, though keeping the style changes I did.

SirVer: From g++ manpage on -Wswitch: "Warn whenever a "switch" statement has an index of enumerated type and lacks a "case" for one or more of the named codes of that enumeration. (The presence of a "default" label prevents this warning.)" which is essentially just rephrasing your conditions.

However, that makes me wonder about -Wswitch-default, which will always trigger if the switch lacks a default case, even if all enumerations are present. Of course, often you don't need a default case when you have defined behaviour for all the enumerations. So at the moment, I can't really see a valid use case it would warn about, which we wouldn't already see from -Wswitch. Do you think I should drop that as well?

As for the "ignore warnings", see also the comments for the other merge requests. The discussion/explanation of this is unfortunately scattered all over the place. :/

However, notice that they are divided into three main groups; basic, additional and gcc. The basic group consists of "-Wall -Wextra" which will be check and applied separately. This means that even if some of the additional warnings arn't supported, those two will always be included. On top of that you migth get a bunch of other additional warnings assuming your compiler supports it, and in the end another small set only supported by GCC. So the only thing you will normally lose out on with an older compiler is the additional and GCC-specific ones, as the minimal set consisting of "-Wall -Wextra" will be there. Naturally I would like to run as many as possible, but historically we haven't run the additional ones at all, so I don't consider it a major flaw. I hope this calms your worries.

Splitting them into sections like this might not be ideal and long-term it would of course be nice to check and apply the warnings individually, however I think it is sufficient for now. I haven't had time to get a good look at the cmake docs, so I don't know if it supports some basic looping to go through a list/array of warnings. Other approaches:
A) check and apply each warning manually. Discarded since it would generate a long list with mostly duplicated code checking different warnings, especially since I would like feedback on which warnings weren't supported/used.
B) introduce som hierarchy checking for compiler name/versions, adding more warnings for higher version numbers. First of all I will rather do feature detection than attempt to sniff for specific names and version numbers, because that strikes me as easier to keep track of. (This is probably because feature detection is the preferred way for web development...

[Nicolai Hähnle (nha)]

Thank you for the change! The style changes are fine.

As for -Wswitch-default, I see what you mean. I have to admit that I'm somewhat torn on the matter. There are basically two arguments, in different directions:

1. Forcing a default: label seems ridiculous if you cover all cases of an enum. This is doubly true because it might actually mask a bug if the enum is later extended. On the other hand, those enums where all cases are treated explicitly most of the time are not really subject to change.

2. Forcing a default: label is an exercise in defensive programming in the sense that it might happen that an enum variable is assigned a value that does not actually appear in the enum. This is of course bad style, but forcing default: labels, and making it a practice to guard them with assertions or even exceptions might flush out such bugs in a more obvious way.

Right now, I lean slightly towards keeping the -Wswitch-default, but it's only a slight preference. In any case, I'm happy with the changes.

review: Approve

[SirVer (sirver)]

lgtm as is. Thanks for working on this!

review: Approve

[Hans Joachim Desserud (hjd)]

Nicolai: Yes, I've tried to weigh the arguments against each other too. I'll keep it in for now, but if we see a lot of false postives in the future, we can of course reconsider and potentially silence it again.

Thanks both, for looking over the changes. Any particular reason why this hasn't been pushed to trunk now that it is approved. Is it implied that I can merge and push it myself? :)

[SirVer (sirver)]

I am quite pressed on time currently, that's why I didn't merge this. And of course - you can do it yourself: two approvals are plenty - besides, the review process should not limit in any way, so just merge when you feel that your work is ready. With review, the code will just be better in general imho.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2013-01-27 14:13:03 +0000
+++ CMakeLists.txt	2013-02-21 19:55:42 +0000
@@ -158,7 +158,7 @@
 set (PARAMETER_COMPILERFLAG_OLDSTYLECAST_EXTENDED "-Wold-style-cast -isystem ${Boost_INCLUDE_DIR}")
 set (PARAMETER_COMPILERFLAG_OLDSTYLECAST "-Wold-style-cast")
 set (PARAMETER_COMPILERFLAG_GENERICWARNINGS "-Wall -Wextra")
-set (PARAMETER_COMPILERFLAG_EXTRAWARNINGS "-Wshadow -Wsign-promo -Wundef -Wunused -Wformat -Wformat-nonliteral -Wformat-security")
+set (PARAMETER_COMPILERFLAG_EXTRAWARNINGS "-Wformat -Wformat-nonliteral -Wformat-security -Wformat-y2k -Winit-self -Winvalid-pch -Wmissing-include-dirs -Woverlength-strings -Wpacked -Wpointer-arith -Wredundant-decls -Wshadow -Wsign-promo -Wswitch-default -Wundef -Wunused -Wunused-macros")
 set (PARAMETER_COMPILERFLAG_GCCWARNINGS "-Wlogical-op -Wsync-nand -Wtrampolines")
 set (PARAMETER_COMPILERFLAG_STRICT "-Werror -Wno-error=old-style-cast -Wno-error=deprecated-declarations -fdiagnostics-show-option")
 IF (CMAKE_BUILD_TYPE STREQUAL "Debug")

=== modified file 'src/editor/editorinteractive.cc'
--- src/editor/editorinteractive.cc	2013-02-10 20:07:27 +0000
+++ src/editor/editorinteractive.cc	2013-02-21 19:55:42 +0000
@@ -333,8 +333,9 @@
 bool Editor_Interactive::handle_key(bool const down, SDL_keysym const code) {
 	bool handled = Interactive_Base::handle_key(down, code);
 
-	if (down)
+	if (down) {
 		// only on down events
+
 		switch (code.sym) {
 			// Sel radius
 		case SDLK_1:
@@ -463,7 +464,7 @@
 			break;
 
 		}
-	else
+	} else {
 		// key up events
 		switch (code.sym) {
 		case SDLK_LSHIFT:
@@ -478,7 +479,7 @@
 		default:
 			break;
 		}
-
+	}
 	return handled;
 }
 

=== modified file 'src/minizip/unzip.cc'
--- src/minizip/unzip.cc	2013-01-06 11:02:56 +0000
+++ src/minizip/unzip.cc	2013-02-21 19:55:42 +0000
@@ -1,6 +1,9 @@
 /* Don't want to modify the minizip/zlib sources, so lets silence the warnings */
 #include "compile_diagnostics.h"
 GCC_DIAG_OFF("-Wold-style-cast")
+GCC_DIAG_OFF("-Wswitch-default")
+GCC_DIAG_OFF("-Wunused-macros")
+CLANG_DIAG_OFF("-Wunused-macros")
 
 /*
 ================================================================================

=== modified file 'src/scripting/pluto.cc'
--- src/scripting/pluto.cc	2013-02-10 17:39:24 +0000
+++ src/scripting/pluto.cc	2013-02-21 19:55:42 +0000
@@ -30,7 +30,9 @@
 // Widelands: silence warnings about unused variables, usually because they
 //are only used in conditional asserts
 #include "compile_diagnostics.h"
+GCC_DIAG_OFF("-Wunused-macros")
 GCC_DIAG_OFF("-Wunused-variable")
+CLANG_DIAG_OFF("-Wunused-macros")
 CLANG_DIAG_OFF("-Wunused-variable")
 
 // Forward declarated from lua_impl.h. So we do not need to include it

=== modified file 'src/ui_basic/messagebox.cc'
--- src/ui_basic/messagebox.cc	2013-02-09 23:36:30 +0000
+++ src/ui_basic/messagebox.cc	2013-02-21 19:55:42 +0000
@@ -143,22 +143,22 @@
 
 bool WLMessageBox::handle_key(bool down, SDL_keysym code)
 {
-	if (!down)
+	if (!down) {
 		return false;
+	}
 
-	switch (code.sym)
-	{
-	case SDLK_KP_ENTER:
-	case SDLK_RETURN:
-		pressedYes();
-		pressedOk();
-		return true;
-	case SDLK_ESCAPE:
-		pressedNo();
-		pressedOk();
-		return true;
-	default:
-		return false;
+	switch (code.sym) {
+		case SDLK_KP_ENTER:
+		case SDLK_RETURN:
+			pressedYes();
+			pressedOk();
+			return true;
+		case SDLK_ESCAPE:
+			pressedNo();
+			pressedOk();
+			return true;
+		default:
+			return false;
 	}
 
 	return UI::Panel::handle_key(down, code);

=== modified file 'src/ui_fsmenu/options.cc'
--- src/ui_fsmenu/options.cc	2013-02-09 23:36:30 +0000
+++ src/ui_fsmenu/options.cc	2013-02-21 19:55:42 +0000
@@ -355,10 +355,8 @@
 
 bool Fullscreen_Menu_Options::handle_key(bool down, SDL_keysym code)
 {
-	if (down)
-	{
-		switch (code.sym)
-		{
+	if (down) {
+		switch (code.sym) {
 			case SDLK_KP_ENTER:
 			case SDLK_RETURN:
 				end_modal(static_cast<int32_t>(om_ok));
@@ -593,10 +591,8 @@
 
 bool Fullscreen_Menu_Advanced_Options::handle_key(bool down, SDL_keysym code)
 {
-	if (down)
-	{
-		switch (code.sym)
-		{
+	if (down) {
+		switch (code.sym) {
 			case SDLK_KP_ENTER:
 			case SDLK_RETURN:
 				end_modal(static_cast<int32_t>(om_ok));