Fix some compiler warnings
ClosedPublic

Authored by yurchor on Nov 7 2019, 7:31 PM.

Details

Summary

There are several logic mistakes in the current code. Compiler warns about them and this patch tries to fix tnem.

Test Plan

Compiles and installs, XMPP part was not tested.

Diff Detail

Repository
R413 KsirK
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
yurchor created this revision.Nov 7 2019, 7:31 PM
Restricted Application added a subscriber: kde-games-devel. · View Herald TranscriptNov 7 2019, 7:31 PM
yurchor requested review of this revision.Nov 7 2019, 7:31 PM
chehrlic added inline comments.
ksirk/GameLogic/gameautomaton.cpp
2200–2201

better use Q_FALLTHROUGH()

ksirk/GameLogic/goal.h
73

Custom copy ctor but default operator=() ? Doesn't look correct.

ksirkskineditor/onu.cpp
781

later pointSize() (int) is assigned to m_font.size - so why not change m_font.size to be an int?

yurchor marked 3 inline comments as done.Nov 7 2019, 8:02 PM

Fixed. Thanks.

yurchor updated this revision to Diff 69409.Nov 7 2019, 8:03 PM

Remove = operator, rise the Qt requirement to 5.8 to use Q_FALLTHROUGH(), use (int).

aacid added a subscriber: aacid.Nov 9 2019, 7:58 PM

Honestly i'm not convinced touching the iris xmpp code that is a copy of something else is the best of the ideas.

Maybe we can disable the extra warnings for that specific folder?

What do others think?

nhirsl added a subscriber: nhirsl.Nov 10 2019, 4:54 PM

Last time I tried playing ksirk over jabber network was during upgrade to Qt5. At that time it was defunct and Gael removed the label from the game selection panel. It looks like there's still a way to chose it trough menu.
Has that changed in the meantime?

Almost all compiler warnings are coming from this part of code. I tried to upgrade it to upstream version and test it, but since it was still defunct I couldn't verify the change.

There's also note (jabbergameui.ui): Playing over the Jabber network is an experimental feature in KsirK.

Not sure what's the right way to proceed:

  1. Drop support/code for playing over Jabber network (remove obsolete iris/xmpp code from ksirk)
  2. Upgrade to latest upstram (which resolves _all_ compiler warnings in that part of code)
  3. Keep it like this and fix here and there some compiler warnings
yurchor updated this revision to Diff 69542.Nov 10 2019, 5:41 PM

Revert changes in iris until future decision

I agree with these changes.
Is it possible to disable warnings for particular folder and does that make sense at all (since the code is still there and compiling)?

I agree with these changes.

Thanks.

Is it possible to disable warnings for particular folder and does that make sense at all (since the code is still there and compiling)?

This does not make sense for me. These warnings cannot be seen by an ordinary user and they are instructive to see what can be done to enhance the code at the same time.

aacid added a comment.Nov 17 2019, 9:36 PM

About the iris warnings, since @nhirsl seems right and we don't seem to use it, i'd suggest to just do

diff --git a/ksirk/CMakeLists.txt b/ksirk/CMakeLists.txt
index 4d5d2f9..1dcad94 100644
--- a/ksirk/CMakeLists.txt
+++ b/ksirk/CMakeLists.txt
@@ -3,7 +3,7 @@ check_include_files(sys/stropts.h SYS_STROPTS_H_FOUND)
 configure_file(config-ksirk.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-ksirk.h)
 
 add_subdirectory(skins)
-add_subdirectory(iris)
+# add_subdirectory(iris)
 add_subdirectory(icons)
 
 # FIND_PACKAGE(ZLIB REQUIRED)

for now, what do you think?

ksirkskineditor/onu.cpp
781

wouldn't it make more sense to make FontDesc size an int?

It's always used to compare with QFont pointsize or to load from it or from file, so i'm not sure why we'd want to keep it being uint.

yurchor marked an inline comment as done.Nov 17 2019, 10:18 PM

About the iris warnings, since @nhirsl seems right and we don't seem to use it, i'd suggest to just do

diff --git a/ksirk/CMakeLists.txt b/ksirk/CMakeLists.txt
 index 4d5d2f9..1dcad94 100644
 --- a/ksirk/CMakeLists.txt
 +++ b/ksirk/CMakeLists.txt
 @@ -3,7 +3,7 @@ check_include_files(sys/stropts.h SYS_STROPTS_H_FOUND)
  configure_file(config-ksirk.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-ksirk.h)
  
  add_subdirectory(skins)
 -add_subdirectory(iris)
 +# add_subdirectory(iris)
  add_subdirectory(icons)
  
  # FIND_PACKAGE(ZLIB REQUIRED)

for now, what do you think?

This does not compile (linking failed, then wrong includes, etc.). This needs more complicated patching, which is out of the scope of the current patch.

yurchor updated this revision to Diff 69895.Nov 17 2019, 10:19 PM

Make font size int.

About the iris warnings, since @nhirsl seems right and we don't seem to use it, i'd suggest to just do

diff --git a/ksirk/CMakeLists.txt b/ksirk/CMakeLists.txt
 index 4d5d2f9..1dcad94 100644
 --- a/ksirk/CMakeLists.txt
 +++ b/ksirk/CMakeLists.txt
 @@ -3,7 +3,7 @@ check_include_files(sys/stropts.h SYS_STROPTS_H_FOUND)
  configure_file(config-ksirk.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-ksirk.h)
  
  add_subdirectory(skins)
 -add_subdirectory(iris)
 +# add_subdirectory(iris)
  add_subdirectory(icons)
  
  # FIND_PACKAGE(ZLIB REQUIRED)

for now, what do you think?

This does not compile (linking failed, then wrong includes, etc.). This needs more complicated patching, which is out of the scope of the current patch.

It does compile for me :?

http://paste.debian.net/1116702/

About the iris warnings, since @nhirsl seems right and we don't seem to use it, i'd suggest to just do

diff --git a/ksirk/CMakeLists.txt b/ksirk/CMakeLists.txt
 index 4d5d2f9..1dcad94 100644
 --- a/ksirk/CMakeLists.txt
 +++ b/ksirk/CMakeLists.txt
 @@ -3,7 +3,7 @@ check_include_files(sys/stropts.h SYS_STROPTS_H_FOUND)
  configure_file(config-ksirk.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-ksirk.h)
  
  add_subdirectory(skins)
 -add_subdirectory(iris)
 +# add_subdirectory(iris)
  add_subdirectory(icons)
  
  # FIND_PACKAGE(ZLIB REQUIRED)

for now, what do you think?

This does not compile (linking failed, then wrong includes, etc.). This needs more complicated patching, which is out of the scope of the current patch.

It does compile for me :?

http://paste.debian.net/1116702/

But not for me (with cleaned "build" subdirectory):

[100%] Linking CXX executable ksirk
/usr/bin/ld: could not found -liris_ksirk
collect2: error: ld returned 1 exit status
make[2]: *** [ksirk/CMakeFiles/ksirk.dir/build.make:1017: ksirk/ksirk] Error 1
make[1]: *** [CMakeFiles/Makefile2:161: ksirk/CMakeFiles/ksirk.dir/all] Error 2
make: *** [Makefile:141: all] Error 2
aacid accepted this revision.Nov 17 2019, 10:59 PM

right, it was using my distro installed ksirk to fullfill the need for that library :)

Ok, you commit this and i'll propose a branch to not use iris

This revision is now accepted and ready to land.Nov 17 2019, 10:59 PM

hmmm, or maybe i won't propose a patch, the changes needed are much bigger than i hoped :/

This revision was automatically updated to reflect the committed changes.