widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

Hans Joachim Desserud has proposed merging lp:~hjd/widelands/compilation-warnings into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1033615 in widelands: "Consider checking for more warnings when compiling Widelands with WL_EXTRAWARNINGS"
  https://bugs.launchpad.net/widelands/+bug/1033615

For more details, see:
https://code.launchpad.net/~hjd/widelands/compilation-warnings/+merge/141946

Related to the intial investigation of additional warnings in bug 1033615.

* WL_EXTRAWARNINGS doesn't exist anymore. If it is specified, cmake will warn it is an unused parameter. I have the impression very few people are using this, so I would rather move the warnings to the default configuration. I guess it was split out like this because -Wextra used to be extremely noise (several thousand extra lines).

* Warnings are grouped in three categories; generic, extra and gcc. Generic are the base warnings (-Wall -Wextra), extra adds additional warnings and gcc attempts to add warnings only availble in gcc.
Extra doesn't add a lot at the moment, but we might expand this in the future depending on the feedback on the bug report. Gcc has so far only added -Wlogical-op, mainly because it has already found a bug.

It is split this way because if an unknown warning is encountered, none of them is applied. This way, -Wall -Wextra should always be applied, if extra applies we get those warnings too and if it doesn't it will inform the user, and if the gcc warnings applies they are added too. In other words, if some of the extra warnings are not availble due to older compiler version or similar, we will always get a good base set of warnings. One thing I would like to see in the future is some way to check and add warnings individually so we get as many as possible. Maybe we could loop through the parameters sent to the compiler and only keep the ones supported.

This will be a gradual process though, as we figure out which warnings we want to add and which we want to silence. I think this is a good starting point though, and give us something we can build further upon.

I've rebuilt Widelands with both GCC and Clang with this revision, they both behave as expected and I got the same list of warnings as the previous run I uploaded to the warning bug reports.

PS. -Wno-attributes was removed because it didn't have any impact on the outcome. Were these warnings ignored due to false positives in the past?
-- 
https://code.launchpad.net/~hjd/widelands/compilation-warnings/+merge/141946
Your team Widelands Developers is requested to review the proposed merge of lp:~hjd/widelands/compilation-warnings into lp:widelands.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2012-11-30 18:16:47 +0000
+++ CMakeLists.txt	2013-01-04 15:56:24 +0000
@@ -157,8 +157,9 @@
 
 set (PARAMETER_COMPILERFLAG_OLDSTYLECAST_EXTENDED "-Wold-style-cast -isystem ${Boost_INCLUDE_DIR}")
 set (PARAMETER_COMPILERFLAG_OLDSTYLECAST "-Wold-style-cast")
-set (PARAMETER_COMPILERFLAG_GENERICWARNINGS "-Wno-attributes -Wall")
-set (PARAMETER_COMPILERFLAG_EXTRAWARNINGS "-Wextra -Wsign-promo")
+set (PARAMETER_COMPILERFLAG_GENERICWARNINGS "-Wall -Wextra")
+set (PARAMETER_COMPILERFLAG_EXTRAWARNINGS "-Wsign-promo")
+set (PARAMETER_COMPILERFLAG_GCCWARNINGS "-Wlogical-op")
 set (PARAMETER_COMPILERFLAG_STRICT "-Werror -Wno-error=old-style-cast -Wno-error=deprecated-declarations -fdiagnostics-show-option")
 IF (CMAKE_BUILD_TYPE STREQUAL "Debug")
   include(CheckCXXCompilerFlag) #this include should be safe
@@ -198,12 +199,21 @@
   IF (Compiler_generic_warnings_supported)
     set (WL_COMPILERFLAG_GENERICWARNINGS " ${PARAMETER_COMPILERFLAG_GENERICWARNINGS}") #the space is on purpose!
   ENDIF (Compiler_generic_warnings_supported)
-  IF (WL_EXTRAWARNINGS)
-    CHECK_CXX_COMPILER_FLAG(${PARAMETER_COMPILERFLAG_EXTRAWARNINGS} Compiler_extra_warnings_supported)
-    IF (Compiler_extra_warnings_supported)
-      set (WL_COMPILERFLAG_EXTRAWARNINGS " ${PARAMETER_COMPILERFLAG_EXTRAWARNINGS}") #the space is on purpose!
-    ENDIF (Compiler_extra_warnings_supported)
-  ENDIF (WL_EXTRAWARNINGS)
+
+  CHECK_CXX_COMPILER_FLAG(${PARAMETER_COMPILERFLAG_EXTRAWARNINGS} Compiler_extra_warnings_supported)
+  IF (Compiler_extra_warnings_supported)
+    set (WL_COMPILERFLAG_EXTRAWARNINGS " ${PARAMETER_COMPILERFLAG_EXTRAWARNINGS}") #the space is on purpose!
+  ELSE (Compiler_extra_warnings_supported)
+    message("Warning: couldn't set the following compiler options: ${PARAMETER_COMPILERFLAG_EXTRAWARNINGS}. Most likely these options are available in a newer release of your compiler.")
+  ENDIF (Compiler_extra_warnings_supported)
+
+  CHECK_CXX_COMPILER_FLAG(${PARAMETER_COMPILERFLAG_GCCWARNINGS} Compiler_gcc_warnings_supported)
+  IF (Compiler_gcc_warnings_supported)
+    set (WL_COMPILERFLAG_GCCWARNINGS " ${PARAMETER_COMPILERFLAG_GCCWARNINGS}") #the space is on purpose!
+  ELSE (Compiler_gcc_warnings_supported)
+    message("Warning: could not add additional GCC-specific warning options: ${PARAMETER_COMPILERFLAG_GCCWARNINGS}. Most likely you are using a different compiler, like Clang/LLVM.")
+  ENDIF (Compiler_gcc_warnings_supported)
+
 
   IF (WL_STRICT)
     CHECK_CXX_COMPILER_FLAG(${PARAMETER_COMPILERFLAG_STRICT} Compiler_strict_mode_supported)
@@ -215,7 +225,7 @@
 ENDIF (CMAKE_BUILD_TYPE STREQUAL "Debug")
 
 # CMAKE only defines "-g", but we need -DDEBUG also, and we need -DNOPARACHUTE (for SDL) in Debug
-set (CMAKE_CXX_FLAGS_DEBUG "-g -DDEBUG -DNOPARACHUTE${WL_COMPILERFLAG_OLDSTYLECAST}${WL_COMPILERFLAG_GENERICWARNINGS}${WL_COMPILERFLAG_EXTRAWARNINGS}${WL_COMPILERFLAG_STRICT}" CACHE STRING "Set by widelands CMakeLists.txt" FORCE)
+set (CMAKE_CXX_FLAGS_DEBUG "-g -DDEBUG -DNOPARACHUTE${WL_COMPILERFLAG_OLDSTYLECAST}${WL_COMPILERFLAG_GENERICWARNINGS}${WL_COMPILERFLAG_EXTRAWARNINGS}${WL_COMPILERFLAG_GCCWARNINGS}${WL_COMPILERFLAG_STRICT}" CACHE STRING "Set by widelands CMakeLists.txt" FORCE)
 
 #This can be removed if no one uses gcc 4.5.1 or 4.5.2 any more
 IF (CMAKE_COMPILER_IS_GNUCXX)