Clang's -Winconsistent-missing-override and GCC's -Wsuggest-override flags warns about missing Q_DECL_OVERRIDE declared.
The patch aims to solve all these warnings, visible on https://build.kde.org/job/Extragear/job/krusader/job/kf5-qt5%20FreeBSDQt5.11/2/warnings12Result/ and reproducible adding -Wsuggest-override or -Winconsistent-missing-override (not sure, haven't tested with clang) to CMAKE_CXX_FLAGS
Compile fine with C+11 enabled/disabled. No regressions on everyday use.
| Lint Skipped |
| Unit Tests Skipped |
and reproducible adding -Wsuggest-override or -Winconsistent-missing-override (not sure, haven't tested with clang) to CMAKE_CXX_FLAGS
Should we add this into cmake config file to ensure that these flags are always on?
| krusader/DiskUsage/filelightParts/fileTree.h | ||
|---|---|---|
| 341 | Please fix spaces. | |
I updated the diff to v2 to fix spaces as requested.
I would not update the flags or, at most, only when build type is Debug.
It's nice to hear from you again, Davide!
> Should we add this into cmake config file to ensure that these flags are always on?
I would not update the flags or, at most, only when build type is Debug.
Updating the debug flags would probably be a good idea.
For my part, my performed tests went alright. Other people can do their checks. Thanks!
I've tested with clang compiler and no additional flags are needed, on a clean Fedora 28 I used:
CC=/usr/bin/clang CXX=/usr/bin/clang++ cmake ..
without the patch the compiler warns about the missing override and with the patch all warnings are gone.
With GCC, I had to set the compiler flag in CMakeLists.txt
if (CMAKE_COMPILER_IS_GNUCXX)
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -Wsuggest-override" CACHE STRING "" FORCE)
endif()it works, you have to manually set -DCMAKE_BUILD_TYPE=Debug, but is it ok?
If so, I will create v2 of the revision
Why not to update the flags for both debug and release (CMAKE_CXX_FLAGS var)? What's your reasoning? Does it hurt the release build?
I've looked at the Clang behaviour: the -Winconsistent-missing-override flag is always set (Debug, Release and none), so GCC should be set the same. My initial thought was to not users to be worried; on a second thought, every user is a tester too.
The new version works using Kubuntu 18.04. Other people can do their checks. Thanks, Davide! (and Nikita :-) )
More importantly, whenever we make a change after this is merged, we will see the warning and fix the code. We put an effort before v2.7 release to make Krusader warning-free and we should keep it this way. So now the -Wsuggest-override is on the list of warning directives that we track. Thanks Davide (and Toni ;) )!
Confirmed by the build (https://build.kde.org/job/Extragear/job/krusader/job/kf5-qt5%20FreeBSDQt5.11/4/warnings12Result/), 51 warnings fixed and 'missing-override' is not in the remaining 57.
Confirmed by the build (https://build.kde.org/job/Extragear/job/krusader/job/kf5-qt5%20FreeBSDQt5.11/4/warnings12Result/), 51 warnings fixed and 'missing-override' is not in the remaining 57.
Excellent!