There are several logic mistakes in the current code. Compiler warns about them and this patch tries to fix tnem.
Details
- Reviewers
aacid - Group Reviewers
KDE Games - Commits
- R413:dfb023217424: Fix some compiler warnings
Compiles and installs, XMPP part was not tested.
Diff Detail
- Repository
- R413 KsirK
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 18887 Build 18905: arc lint + arc unit
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? |
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?
For the record, this patch fixes problems in s5b.cpp which were earlier fixed upstream:
https://github.com/psi-im/iris/blob/master/src/xmpp/xmpp-im/s5b.cpp#L744
https://github.com/psi-im/iris/blob/master/src/xmpp/xmpp-im/s5b.cpp#L2227
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:
- Drop support/code for playing over Jabber network (remove obsolete iris/xmpp code from ksirk)
- Upgrade to latest upstram (which resolves _all_ compiler warnings in that part of code)
- Keep it like this and fix here and there some compiler warnings
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)?
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.
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 โ | (On Diff #69542) | 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. |
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.
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
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
hmmm, or maybe i won't propose a patch, the changes needed are much bigger than i hoped :/