Getting warnings in MinGW - SFML Book - Source Code on Github

[Max]

Getting warning in MinGW due to unordered initialized members.
Here is Aircraft.cpp/hpp as an example:

$ make
Windows makefile running...
Compiling...
g++ -Wall -std=c++11 -c -s Aircraft.cpp -o ../Temp/Aircraft.o
In file included from Aircraft.cpp:1:0:
Aircraft.hpp: In constructor 'Aircraft::Aircraft(Aircraft::Type, const TextureHolder&, const FontHolder&)':
Aircraft.hpp:76:12: warning: 'Aircraft::mSpawnedPickup' will be initialized after [-Wreorder]
Aircraft.hpp:67:17: warning:   'sf::Sprite Aircraft::mSprite' [-Wreorder]
Aircraft.cpp:23:1: warning:   when initialized here [-Wreorder]
In file included from Aircraft.cpp:1:0:
Aircraft.hpp:89:12: warning: 'Aircraft::mIdentifier' will be initialized after [-Wreorder]
Aircraft.hpp:77:12: warning:   'bool Aircraft::mPickupsEnabled' [-Wreorder]
Aircraft.cpp:23:1: warning:   when initialized here [-Wreorder]
 

Following files are affected:

Aircraft.cpp
GameServer.cpp
Player.cpp
 

Also there is one more warning to get rid of:

Utility.cpp: In function 'std::string toString(sf::Keyboard::Key)':
Utility.cpp:28:9: warning: enumeration value 'KeyCount' not handled in switch [-Wswitch]
 

Change from:

                BOOK_KEYTOSTRING_CASE(F13)
                BOOK_KEYTOSTRING_CASE(F14)
                BOOK_KEYTOSTRING_CASE(F15)
                BOOK_KEYTOSTRING_CASE(Pause)
        }

        return "";
}
 

Change this to:

                BOOK_KEYTOSTRING_CASE(F13)
                BOOK_KEYTOSTRING_CASE(F14)
                BOOK_KEYTOSTRING_CASE(F15)
                BOOK_KEYTOSTRING_CASE(Pause)
                default:
                        return "";
        }

        return "";
}
 

Please update this source code on github. I know it is only warnings but I find them annoying.

[eXpl0it3r]

Please update this source code on github. I know it is only warnings but I find them annoying.

Well make a pull request to ease the process!

[Max]

Okay I could try, never done that before.

[Nexus]

Thanks, I'll have a look at the warnings. By the way, pull requests on GitHub are rather annoying, at least for auto-merging.

Edit: I've just taken a deeper look at it. Using -Wall yields a ton of meaningless warnings related to switch and missing enumerators. For example, every time we handle events, a warning is emitted for every event single type that is not explicitly handled.

Fixing the warnings requires useless default: labels in all of these switch statements, which is not only annoying, but makes code less readable and differ from the snippets in the book for no obvious reason. I really don't like complicating perfectly valid C++ code. You can use -Wno-switch to disable this warning.

However I'll have a look at the other warnings, such as initialization list orders.

[Nexus]

There are also other warnings that are not justified:

template <typename Resource, typename Identifier>
void ResourceHolder<Resource, Identifier>::insertResource(Identifier id, std::unique_ptr<Resource> resource)
{
        // Insert and check success
        auto inserted = mResourceMap.insert(std::make_pair(id, std::move(resource)));
        assert(inserted.second);
}

warning: variable 'inserted' set but not used [-Wunused-but-set-variable]

This is a flaw in g++. They should rather implement assert in a way such that with NDEBUG defined, the expression has no effect, but counts as "used".

[Laurent]

They should rather implement assert in a way such that with NDEBUG defined, the expression has no effect, but counts as "used".

It's not as simple. If your expression has side effects, the side effects must disappear in release build, so the expression must really disappear in release build, and thus there's no way it can be "used".

Another solution would be to explicitely mark the variable as unused in your code ((void)inserted, or through a UNUSED(x) macro). This says explicitly that the variable is unused in your code, and just declared for debug checks.

[Nexus]

It's not as simple. If your expression has side effects, the side effects must disappear in release build, so the expression must really disappear in release build, and thus there's no way it can be "used".

I'm sure compiler and standard library writers would be able to come up with a solution, maybe even something simple like sizeof(expr) or decltype(expr) would work.

Another solution would be to explicitely mark the variable as unused in your code ((void)inserted, or through a UNUSED(x) macro).

Yes, but it's still redundant. And imagine the book readers stumbling about something like (void)inserted, it will raise a lot of questions.

That's exactly what I want to avoid: Complicating the code or changing it so that it doesn't match the snippets in the book anymore. I personally consider the warnings (which aren't active by default, only through -Wall) less severe than the possible confusion. Since this code exists for teaching purpose, clarity and simplicity should be the focus -- not complicated workarounds to satisfy different compilers. It's bad enough we had to come up with a FOREACH macro to emulate range-based for on VS 2010

[Hiura]

Fixing the warnings requires useless default: labels in all of these switch statements, which is not only annoying, but makes code less readable and differ from the snippets in the book for no obvious reason. I really don't like complicating perfectly valid C++ code.

I kindly disagree here. 

You can get undefined behaviour, or «simply» vicious bugs when the default case is missing. Here is a dummy example where the RIGHT case is missing. Pretty painful to debug.

switch (direction) {
  case UP: x = 1; break;
  case DOWN: x = 2; break;
  case LEFT: x = 3; break;
}
 

Or if you need to return some value, it's best (IMO) to use `default: return x;` than returning x after the switch:

switch (y) {
  default: return x;
  case A: return a;
...
}

Regarding your assertion, wouldn't it be the same to `assert(resource)` before the insertion? I fail to understand why you want to test it after..