Fix archlinux compilation by fixing error-warnings `ignored qualifiers` and `implicit fallthrough` #3270

berserkingyadis · 2018-05-31T18:24:07Z

Related Ticket(s)

Fixes Compilation on Archlinux fails because of missing break statements #3240

Short roundup of the initial problem

Compiling the newest Cockatrice fails under Archlinux. These are the cmake options I have used:

-DCMAKE_BUILD_TYPE=Debug -DWITH_SERVER=1

Output:
https://hastebin.com/ajahasacow.rb
https://hastebin.com/esigarajun.vbs

What will change with this Pull Request?

The code will be fixed, so that these warnings will no longer occur.
Archlinux will be able to compile in Debug mode

Daenyth · 2018-05-31T18:31:28Z

How bad is it to fix ignored-qualifiers rather than ignoring it?

berserkingyadis · 2018-05-31T18:57:29Z

Ill post the ignored-qualifiers warnings in here, dunno how many there are.

berserkingyadis · 2018-05-31T19:27:02Z

There are 2 warnings each.

implicit-fallthrough:

/home/bers/Projects/contrib/Cockatrice/cockatrice/src/gamesmodel.cpp: In member function ‘virtual QVariant GamesModel::headerData(int, Qt::Orientation, int) const’:
/home/bers/Projects/contrib/Cockatrice/cockatrice/src/gamesmodel.cpp:194:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
             switch (role) {
             ^~~~~~
/home/bers/Projects/contrib/Cockatrice/cockatrice/src/gamesmodel.cpp:201:9: note: here
         case DESCRIPTION:
         ^~~~
/home/bers/Projects/contrib/Cockatrice/cockatrice/src/gamesmodel.cpp:210:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
             switch (role) {
             ^~~~~~
/home/bers/Projects/contrib/Cockatrice/cockatrice/src/gamesmodel.cpp:217:9: note: here
         case SPECTATORS:
         ^~~~

(only 2 in the whole project?)

ignored-qualifiers:

/home/bers/Projects/contrib/Cockatrice/cockatrice/src/carditem.cpp: In member function ‘virtual void CardItem::mouseMoveEvent(QGraphicsSceneMouseEvent*)’:
/home/bers/Projects/contrib/Cockatrice/cockatrice/src/carditem.cpp:320:89: warning: type qualifiers ignored on cast result type [-Wignored-qualifiers]
             const ZoneViewZone *const view = static_cast<const ZoneViewZone *const>(zone);
                                                                                         ^
/home/bers/Projects/contrib/Cockatrice/cockatrice/src/tab_game.cpp: In member function ‘void TabGame::eventPlayerPropertiesChanged(const Event_PlayerPropertiesChanged&, int, const GameEventContext&)’:
/home/bers/Projects/contrib/Cockatrice/cockatrice/src/tab_game.cpp:1032:81: warning: type qualifiers ignored on cast result type [-Wignored-qualifiers]
         static_cast<const GameEventContext::ContextType>(getPbExtension(context));
                                                                                 ^

Seeing that there are only 2 warnings each, one could fix these imo.

(all warnings when compiling: https://pastebin.com/tfxaitET)

berserkingyadis · 2018-06-02T11:59:18Z

I do not have the knowledge to fix the ignored-qualifiers warnings, can somebody help?

Daenyth · 2018-06-02T14:24:54Z

I think it's just removing some of the const. @ctrlaltca ?

ctrlaltca · 2018-06-05T12:25:05Z

This could use the workaround of #3275.
Of course the real solution is to fix the code; sorry but i have no gcc 8.1 available yet to test.

berserkingyadis · 2018-06-05T16:32:05Z

I fixed the 2 warnings for ignored-qualifiers and added an exception for implicit-fallthrough, since break-less switches are used intentionally (#3240 (comment)).

Is this PR ok like this?

berserkingyadis · 2018-06-05T16:36:23Z

cockatrice/src/carditem.cpp

@@ -317,7 +317,7 @@ void CardItem::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
            2 * QApplication::startDragDistance())
            return;
        if (zone->getIsView()) {
-            const ZoneViewZone *const view = static_cast<const ZoneViewZone *const>(zone);
+            const ZoneViewZone *view = static_cast<const ZoneViewZone *>(zone);


I removed the const from the pointer since the type of zone is non-const pointer.

Cockatrice/cockatrice/src/carditem.h

Line 23 in 89ab7f1

CardZone *zone;

that should have been the issue.

ctrlaltca · 2018-06-05T16:49:56Z

c++17 adds a [[fallthrough]] statement that can be used to inform the compiler that the fallthrough is intentional.
Since we currently only support c++11, only compiler-specific statements can be used. Here's how mozilla is dealing with these, maybe it can be an inspiration:

#ifndef __has_cpp_attribute
#  define __has_cpp_attribute(x) 0
#endif

#if __has_cpp_attribute(clang::fallthrough)
#  define MOZ_FALLTHROUGH [[clang::fallthrough]]
#elif __has_cpp_attribute(gnu::fallthrough)
#  define MOZ_FALLTHROUGH [[gnu::fallthrough]]
#elif defined(_MSC_VER)
    /*
     * MSVC's __fallthrough annotations are checked by /analyze (Code Analysis):
     * https://msdn.microsoft.com/en-us/library/ms235402%28VS.80%29.aspx
     */
#  include <sal.h>
#  define MOZ_FALLTHROUGH __fallthrough
#else
#  define MOZ_FALLTHROUGH /* FALLTHROUGH */
#endif

usage

switch(test)
{
	case 1:
		break;
	case 2:
		MOZ_FALLTHROUGH
	default:
		break;
}

berserkingyadis · 2018-06-05T17:10:21Z

where should I add the MOZ_FALLTHROUGH macro? a new header file?

ctrlaltca · 2018-06-05T17:11:58Z

a new header file?

Yep, that's the idea, and then #include "newfile.h" only where needed

berserkingyadis · 2018-06-05T18:01:04Z

That is working. (Sorry for the many edits)

We should credit the mozilla guys for the macro right?

berserkingyadis · 2018-06-05T20:49:43Z

That should do it, the macro should be tested on other platforms (clang, osx, windows)

Both warnings are fixed in the code, CMakeLists.txt remains untouched.

ctrlaltca · 2018-06-06T06:46:27Z

cockatrice/src/gamesmodel.cpp

@@ -197,6 +198,7 @@ QVariant GamesModel::headerData(int section, Qt::Orientation /*orientation*/, in
                case Qt::TextAlignmentRole:
                    return Qt::AlignCenter;
            }
+            SWITCH_FALLTHROUGH;


I'm sorry i didn't look at the code before bringing up the MOZ_FALLTHROUGH topic, but here a simple break; would have avoided the warning without the need of adding a new header file.
To be precise, we don't want it to fall through.

If the other switch statement is the same I could just add a break there too. Remove the header file?

yes, sorry again for the wasted time

adding a break in these places makes the compiler think that the control could reach the end of the function error: control reaches end of non-void function [-Werror=return-type]

since the switch defaults to return QVariant(); I think ill add it at the end of the function

another idea would be adding default cases to both the switches returning QVariant(), this is done alot in the function above

or, you can add return QVariant() in place of the breaks

…gnored qualifiers

berserkingyadis · 2018-06-06T12:12:31Z

ok, added the defaults to the switches

tooomm · 2018-06-07T06:12:42Z

Can you update your starting post? @berserkingyadis

berserkingyadis · 2018-06-08T10:53:33Z

updated, should reflect the changes now.

ctrlaltca · 2018-06-09T08:28:26Z

Merged, thank you!

berserkingyadis commented Jun 5, 2018

View reviewed changes

berserkingyadis changed the title ~~[WIP] Fix archlinux compilation by adding warning-error exceptions~~ Fix archlinux compilation by adding warning-error exceptions Jun 5, 2018

ctrlaltca reviewed Jun 6, 2018

View reviewed changes

fixed 2 warning-errors on archlinux(gcc 8.1): implicit fallthrough, i…

d634694

…gnored qualifiers

berserkingyadis changed the title ~~Fix archlinux compilation by adding warning-error exceptions~~ Fix archlinux compilation by fixing error-warnings ignored qualifiers and implicit fallthrough Jun 7, 2018

Daenyth approved these changes Jun 8, 2018

View reviewed changes

ctrlaltca merged commit 91b3c73 into Cockatrice:master Jun 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix archlinux compilation by fixing error-warnings `ignored qualifiers` and `implicit fallthrough` #3270

Fix archlinux compilation by fixing error-warnings `ignored qualifiers` and `implicit fallthrough` #3270

berserkingyadis commented May 31, 2018 •

edited

Loading

Daenyth commented May 31, 2018

berserkingyadis commented May 31, 2018

berserkingyadis commented May 31, 2018

berserkingyadis commented Jun 2, 2018

Daenyth commented Jun 2, 2018

ctrlaltca commented Jun 5, 2018

berserkingyadis commented Jun 5, 2018 •

edited

Loading

berserkingyadis Jun 5, 2018

ctrlaltca commented Jun 5, 2018 •

edited

Loading

berserkingyadis commented Jun 5, 2018

ctrlaltca commented Jun 5, 2018

berserkingyadis commented Jun 5, 2018 •

edited

Loading

berserkingyadis commented Jun 5, 2018

ctrlaltca Jun 6, 2018 •

edited

Loading

berserkingyadis Jun 6, 2018

ctrlaltca Jun 6, 2018

berserkingyadis Jun 6, 2018

berserkingyadis Jun 6, 2018

ctrlaltca Jun 6, 2018

berserkingyadis commented Jun 6, 2018

tooomm commented Jun 7, 2018

berserkingyadis commented Jun 8, 2018

ctrlaltca commented Jun 9, 2018

Fix archlinux compilation by fixing error-warnings ignored qualifiers and implicit fallthrough #3270

Fix archlinux compilation by fixing error-warnings ignored qualifiers and implicit fallthrough #3270

Conversation

berserkingyadis commented May 31, 2018 • edited Loading

Related Ticket(s)

Short roundup of the initial problem

What will change with this Pull Request?

Daenyth commented May 31, 2018

berserkingyadis commented May 31, 2018

berserkingyadis commented May 31, 2018

berserkingyadis commented Jun 2, 2018

Daenyth commented Jun 2, 2018

ctrlaltca commented Jun 5, 2018

berserkingyadis commented Jun 5, 2018 • edited Loading

berserkingyadis Jun 5, 2018

Choose a reason for hiding this comment

ctrlaltca commented Jun 5, 2018 • edited Loading

berserkingyadis commented Jun 5, 2018

ctrlaltca commented Jun 5, 2018

berserkingyadis commented Jun 5, 2018 • edited Loading

berserkingyadis commented Jun 5, 2018

ctrlaltca Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

berserkingyadis Jun 6, 2018

Choose a reason for hiding this comment

ctrlaltca Jun 6, 2018

Choose a reason for hiding this comment

berserkingyadis Jun 6, 2018

Choose a reason for hiding this comment

berserkingyadis Jun 6, 2018

Choose a reason for hiding this comment

ctrlaltca Jun 6, 2018

Choose a reason for hiding this comment

berserkingyadis commented Jun 6, 2018

tooomm commented Jun 7, 2018

berserkingyadis commented Jun 8, 2018

ctrlaltca commented Jun 9, 2018

Fix archlinux compilation by fixing error-warnings `ignored qualifiers` and `implicit fallthrough` #3270

Fix archlinux compilation by fixing error-warnings `ignored qualifiers` and `implicit fallthrough` #3270

berserkingyadis commented May 31, 2018 •

edited

Loading

berserkingyadis commented Jun 5, 2018 •

edited

Loading

ctrlaltca commented Jun 5, 2018 •

edited

Loading

berserkingyadis commented Jun 5, 2018 •

edited

Loading

ctrlaltca Jun 6, 2018 •

edited

Loading