Warnings at compile-time (clang/llvm)

[Hans Joachim Desserud]

First of all, note that builds compiled with clang has some problems and will currently crash (see bug 744595 for details). That is naturally a lot more severe issue which should be sorted out, before we can start using clang normally.

However, compiling with clang will output a ton of warnings (a lot more than GCC). I realize in the current state it looks rather noisy (more than 27000 lines), however I believe that fixing these warnings will sort out issues in the code, and also make it easier to spot when new warnings are introduced by new code.

At a glance there seem to be a couple of issues which seem to trigger the most warnings:
* Mixed use of struct/class. Declared as one, but refered to as the other in another place. I suspect this could be easily fixed by a) checking what it is declared as in the first place and b) run some combination of find/sed replacing all occurances of "struct name" with "class name" or vice versa.
* Virtual functions are overloaded.
* Several other minor issues

I suspect there is a lot of low-hanging fruit in here, and it might be suitable new contributors. Though note that since clang is not used currently, so consider this EXTREMELY LOW priority. Still, it would always be nice to reduce the amount of warnings :)

PS. See also bug 825957 for warnings reported by GCC, and bug 986611 for issues reported by cppcheck.

Some technical info:

Widelands bzr r6183 compiled with Clang (LLVM) 3.0-1 on Arch.

Compiled with the following options
 cmake ../widelands -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=/opt/widelands-clang/build/ -DWL_PORTABLE=true -DWL_EXTRAWARNINGS=true -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++

Note that -DWL_EXTRAWARNINGS include -Wextra in addition to -Wall which is enabled by default.

[Hans Joachim Desserud (hjd)]

Updated list of warnings. High five to aber for reducing it by 20.000 lines :)

[Peter Waller (peter.waller)]

With the merge in #6394, the huge majority of these warnings have now been killed!

Attached is the remaining 93 lines of warning output. (Including the lines only containing a caret pointing at the issue).

These ones in particular are bugs that need fixing:

src/md5.h:94:7: warning: 'MD5Checksum<basic_FileRead<StreamRead> >::Data' hides overloaded virtual function [-Woverloaded-virtual]
src/ui_basic/checkbox.cc:145:23: warning: & has lower precedence than !=; != will be evaluated first [-Wparentheses]
    if (oldhl != m_flags & Is_Highlighted)
src/logic/player.cc:1078:22: warning: variable 'id' is uninitialized when used here [-Wuninitialized]
    for (uint32_t id; id < stocks.size(); ++id) {

It would be nice to also get rid of the unused variable warnings, for this it may be necessary to add an UNUSED() macro to declare intention that a variable should be left unused. Or alternatively to rewrite loops which don't use the automatically declared variable to just iterate using a for(i = 0; i < n_things; i++) loop instead.

[SirVer (sirver)]

Much obliged Peter! That was a lot of work you did there.

I am no fan of UNUSED macros and would suggest rewriting loops instead.
In C++ is is also valid to leave the name of a method argument out which
should also help with some warnings.

[Peter Waller (peter.waller)]

My pleasure, SirVer.

I thought about them and independently realised that UNUSED was generally a bad idea.

The most recent merge proposal fixes them without such a macro, but it does introduce is_a, instead.

[SirVer (sirver)]

I like the is_a approach. Reads very naturally in the code imho. I merged your latest branch in 6399.

[Hans Joachim Desserud (hjd)]

Up to date report based on current trunk.

[Hans Joachim Desserud (hjd)]

Clang 3.2 was recently released, so here is a new status update.

Mainly complains about unused/unitialized variables, and some variables which might end up uninitialized depending on the circumstances. (Not sure if the latter is new in the code or a new check in the compiler)

[Hans Joachim Desserud (hjd)]

After GCC is now warning free, it is no wonder this also reduced the amount of warnings for other compilers. Some highlights:
-some methods are hidden
-various unused private members (I suppose these can simple be removed?)
-an unused variable in pluto (though since this is clang, it won't register if it is silenced for gcc. Hm...)

[Mark Scott (mxsscott)]

The unused variable is used if lua_assert() is active.

Similar #define magic can be placed in compile_diagnostic.h to create a CLANG_DIAG_OFF(x) and CLANG_DIAG_ON(x) if that's how clang works, and then the top of pluto's file can say:

    GCC_DIAG_OFF("-Wunused-variable")
    CLANG_DIAG_OFF("WhateverClangNeeds")

(Pluto here being an exception that we just nuke the warning for the whole file).

For the others, we should of course fix the issue. I'll install clang 3.2 and have an attack at them.

BTW, once warning free I would consider adding the GCC option "-Werr", for at least GCC 4.6, to treat warnings as compile errors which will terminate the build.

[Hans Joachim Desserud (hjd)]

I thought about -Werror initially, but I am not convinced. It would make it more painful when dealing with newer (read:stricter) versions of compilers, porting to different OSes, adding new compilers etc. Now, I realize most of these are edge cases, but I don't really want to punish people who just want to run WL in a different way. It seems more straight-forward to me that people who would run into these kinds of problems can simply compile with the default settings and see some warnings printed rather than having to look up and use some "allow-warnings" flag.

Btw, we have WL_STRICT as a compilation argument to enable -Werror (and a couple of others I didn't check).

[SirVer (sirver)]

-Werror is not a good idea for open source. In a corp environment, you can make sure that everybody uses the same compiler and libraries and then every warning is essentially an error and -Werror is a good thing. But in open source, people use so many different compilers, libraries and os'es that it is impossible to guarantee no errors for every computer - e.g. my wife compiles widelands on her old mac using gcc-4.2 which barfs at __attribute__(hot) - Werror would make it hard for her to compile at all.

[Mark Scott (mxsscott)]

All known warnings are now fixed, silent compilation is now possible with gcc-4.7 & clang/llvm-3.2 on my system.

[SirVer (sirver)]

Released in build-18 rc1.