Merge lp:~ab-tools/widelands/msvs2010-compilewarnings into lp:widelands

You make a casts to float for a lot of gl...f() (e.g. glVertex2f(static_cast<float>(integer))) functions, when you could instead use the gl...i (integer) functions and not require the cast.

Hello Peter,

first thanks for your comment!

Yes, I thought the same and tried to use the "gl...i" functions instead, but as a result the OpenGL rendering did not work anymore correctly (the whole menu turned black after doing this change and trying to switch to OpenGL rendering mode).

I have to admit that I'm not familiar with the whole rendering staff, so I don't know why that's the case (maybe calculation precision issues when using the integer functions?), but it was what I experienced when running Widelands on Windows.

Maybe someone else can explain why that's the case or you can also test it on Linux if it works for you if you use the integer functions instead.

I get a some compiler errors concerning _strdup. It is not defined on my system.

I am not too fond with the arbitrary type casting all around. The proper fix would be to track down where the wrong types are used and fixing this throughout the code. If a type is necessary it should only be done exactly once. I feel uneasy just casting around to shut up the compiler like that.

_strdup is vs2010 way of saying that it's not strictly ansi c standard function. In similar cases I have silenced the compiler by a compiler specific define in top level CMakeLists.txt.

That's OK, then I'll revert the "underscore" function name changes and add a Compile directive to the CMake file.

Regarding the typecasting, I'll check if there are possibilities to use better fitting variable types to reduce typecasting.

Hi, thanks for taking your time working on eliminating warnings and improving code quality.

Since this merge request was filed, a lot of things have changed in the code, including plenty of warnings reported by GCC and Clang have been addressed. Effectively, this means that applying these changes might trigger conflicts and that some of the warnings might already have been addressed.

Don't get me wrong though, since every compiler is implemented differently, they likely report different amounts and types of warnings. So while fixing warnings in GCC/Clang may also have improved the situation in VS, we would still like any warnings only found in VS to be fixed. I would be really happy if WL could compile warning-free in the various compilers we use.

Could you please take a look at how the current trunk looks in Visual Studio and or see what happens if you attempt to merge latest trunk into this branch. I fear you are going to run into conflicts when attempting the latter though.

First thanks for your reply, Hans!

I retested now to compile the very latest BZR version (without any changes) under Windows, but there is no chance right now due to a lot of new compile errors (not warnings).

The most errors are referring to a "Syntax error: '('" in these two lines:
- "panel.h": line 250: void do_draw(RenderTarget &) __attribute__((hot));
- "button.h": line 72: void draw(RenderTarget &)__attribute__((hot));

Any ideas on that?

Best regards
Andreas

Sorry, I only know basic c++, so I'm not the right person to answer that. :P

This obviously needs to be sorted out before we can proceed with the warnings though. Could you file a bug report on this and tag it 'regression'?

Same for me as I'm more on the C# (.NET) side normally. ;-)

Therefore I've created a bug report now as you suggested:
https://bugs.launchpad.net/widelands/+bug/1142781

Sorry for not following up on this before now, but now that bug 1142781 is fixed, what is the current status on warnings in VS?

To be honest I didn't test it again for quite a long time and there were a lot of changes in the code since then.

But I'll give it a try again. I think I should find some time for a new compilation test on Windows this weekend and post some feedback afterwards.

I've done a new compiling test on Windows (using Visual Studio 2010). Compilation worked, but again with a lot of warnings.

These two warnings were thrown already by CMake:
http://www.ab-tools.com/temp/Widelands-CMakeWarnings.txt
These probably can be fixed easily by adding an if clause around the lines "set (PARAMETER_COMPILERFLAG_EXTRAWARNINGS*" and "set (PARAMETER_COMPILERFLAG_GCCWARNINGS*" in the "CMakeList.txt" file.

And here is the list of warnings by the Visual Studio compiler:
http://www.ab-tools.com/temp/Widelands-CompilerWarnings.txt
As you can see many of these warnings are the same and can be fixed in one go (e. g. the "warning C4086: Pragma-Parameter"). But as I'm not the most experienced C++ developer (im coming from the C# side ;-) ), I need some help here to make the right changes.

So if you have any suggestions, please just let me know and I'm happy to test it here.

There are a lot, so this list is hard to access. Let's take this in small steps then.

I tried fixing the error in c:\widelands\widelands\src\logic\immovable.h:163 in r6618. And i tried to fix the pragma warning in field.h in r6619. Could you redo the list?

Regarding the CMake warnings, check CMakeList.txt lines 206 and 213. They are in if clauses already. If the compiler supports the additional warnings, they are appended to the options used, otherwise the warnings you saw are printed. This was done both to explain why the additional warnings aren't used and to avoid silently using less without being told why. I guess I could somehow improve the warnings to make this a bit clearer, at least mention Visual Studio as an alternative in the GCC-only warning.

Yes, that really helped already!

Now there are only 100 warnings instead of almost 750 left:
http://www.ab-tools.com/temp/Widelands-CompilerWarnings2.txt

Too happy too soon: for whatever reason Visual Studio did not show all warnings...

Now I have the full list - 442 left and I updated the list here again:
http://www.ab-tools.com/temp/Widelands-CompilerWarnings2.txt

Anyway, better already than the 742 in the first compilation today. ;-)

Okay, there are still a bunch of dups in this one - most of them will need manual intervention to get fixed properly.

If you are still willing to fix them, I can review them. But I would much appreciate if we could take them in smaller merge requests - this one is too much to chew on right now for me, and I am not sure how many merge conflicts this branch already has. Is this somehow possible for you without much work?

Yes, this merge request has not much to do with the current warnings anymore as it is very old already.

As I wrote to Hans few days ago I am very happy to help with doing compilation tests here on Windows, but I won't be able to provide many fixes myself for most of these issues as I do have too less C++ experience (as a C# developer).

But I could open a bug report for these warnings if you prefer to go on their instead of in this merge request (which is no longer really useful).

Anyway, I at least have a patch for the 40 warnings regarding not referenced variables:
http://www.ab-tools.com/temp/Widelands-NotReferencedVariables.patch
You're welcome to commit that directly.

After that the compiler warnings are reduced to only 402: ;-)
http://www.ab-tools.com/temp/Widelands-CompilerWarnings3.txt

Okay, I applied the patch. I think we should talk about the rest of the warnings in a bug report. First aim should be to make the warning options similar to what we use for the gnu tools (for example we do not care about signed vs unsigned comparison there). Some of the warnings look more serious and are worth investigating for sure, i.e. the unary minus on unsigned values and the overwritten warnings.

Mind opening a bug report for this? We can take it step by step. In the meantime, I set this one to rejected as it is no longer a real merge request. Sounds good?

That's OK, and I also reported the bug here:
https://bugs.launchpad.net/widelands/+bug/1202101

In the current BZR version compilation on Windows is even not possible at all as there are also compile errors now.