fix some clang/gcc warnings found integrating qtpromise

[ssproessig]

In file included from xxx.cpp:19:
In file included from /home/ssp/src/qtpromise/include/QtPromise:11:
In file included from /home/ssp/src/qtpromise/include/../src/qtpromise/qpromise.h:11:
/home/ssp/src/qtpromise/include/../src/qtpromise/qpromise_p.h:116:26: warning: function 'rethrow' could be declared with attribute 'noreturn' [-Wmissing-noreturn]
    void rethrow() const { std::rethrow_exception(m_data); }
                         ^
/home/ssp/src/qtpromise/include/../src/qtpromise/qpromise_p.h:55:9: warning: '~Event' overrides a destructor but is not marked 'override' [-Wsuggest-destructor-override]
        ~Event() { m_f(); }
        ^
/home/ssp/src/qtpromise/include/../src/qtpromise/qpromise_p.h:51:12: note: in instantiation of member class 'Event' requested here
    struct Event : public QEvent
           ^
/home/ssp/src/qtpromise/include/../src/qtpromise/qpromise_p.h:542:13: note: in instantiation of function template specialization 'QtPromisePrivate::qtpromise_defer<const std::function<void ()> &>' requested here
            qtpromise_defer(handler.second, handler.first);
            ^
/qt/5.15.1/gcc_64/include/QtCore/qcoreevent.h:299:13: note: overridden virtual function is here
    virtual ~QEvent();
            ^

[simonbrunel]

Thanks @ssproessig! It would be a good idea to add these warnings to the project compile options, so we will not forget about them next time.

[ssproessig]

...regarding the compiler options: in our projects we usually use the following switches -besides -Wdocumentation in Clang- none of them brought up further problems. Nevertheless, we distinguish between Clang and GCC, especially for GCC we follow the Red Hat Secure Flags ( -Wformat=2 -Wformat-security -fstack-protector-all)

# Options for different compilers are defined in this file (on project-level).

list(APPEND COMPILE_OPTIONS_GCC
    -Wall
    -Wextra
    -Wpedantic
    
    -Wconversion
    -Wdouble-promotion
    -Wduplicated-branches
    -Wduplicated-cond
    -Wlogical-op
    -Wnon-virtual-dtor
    -Wold-style-cast
    -Wshadow
    -Wstrict-overflow
    -Wuseless-cast
    
    -Wformat=2
    -Wformat-security
    -fstack-protector-all
    
    -fdiagnostics-color=always
)

# https://quuxplusone.github.io/blog/2018/12/06/dont-use-weverything/
list(APPEND COMPILE_OPTIONS_CLANG
    -Wall
    -Wextra
    -Wpedantic

    -Weverything
    -Wno-switch-enum -Wswitch -Wswitch-default
    -Wno-weak-vtables
    -Wno-padded
    -Wno-packed
    -Wno-c++98-compat-pedantic
    -Wno-c++98-compat
    -Wno-return-std-move-in-c++11
    -Wno-exit-time-destructors
    -Wno-missing-prototypes
    -Wno-documentation-unknown-command
    -Wno-used-but-marked-unused
    -Wno-gnu-zero-variadic-macro-arguments
    -Wno-disabled-macro-expansion
    -Wno-global-constructors

    -Wformat-pedantic
    
    -fcolor-diagnostics
)

list(APPEND COMPILE_OPTIONS_MSVC
    /W4
    /experimental:external /external:anglebrackets /external:W0
)

list(APPEND LINK_OPTIONS_MSVC
    /debug:fastlink
)


#refer to: https://docs.microsoft.com/en-us/cpp/build/reference/jmc?view=vs-2019
set(CMAKE_VS_JUST_MY_CODE_DEBUGGING $<$<CONFIG:Debug>:ON>)

Shall I add any others besides -Wshadow and -Wsuggest-destructor-override (coming from -Weverything in Clang, which is rather "controversial")?

[simonbrunel]

Let's focus on GCC since the CI doesn't build with Clang (yet).

While I'm not sure which ones are available since GCC 4.9, I would try to introduce only these additional flags:

-Wdouble-promotion
-Wduplicated-branches
-Wduplicated-cond
-Wformat=2
-Wlogical-op
-Wshadow
-Wswitch-default
-Wmissing-noreturn
-Wuseless-cast

I can't find a GCC equivalent for -Wsuggest-destructor-override.

About other suggested flags:

-Wformat-security      // part of -Wformat=2
-Wnon-virtual-dtor     // can't find docs about this one
-Wstrict-overflow      // part of -Wall
-Wswitch               // part of -Wall

[simonbrunel]

Feel free to drop the -Wshadow one since it seems to cause too many errors (I will fix it later). Also, let's drop the ones not compatible with GCC 4.9 (-Wduplicated-branches & -Wduplicated-cond).

[ssproessig]

Feel free to drop the -Wshadow one since it seems to cause too many errors (I will fix it later). Also, let's drop the ones not compatible with GCC 4.9 (-Wduplicated-branches & -Wduplicated-cond).

as always - "it's only a small change" hangs in CI/CD - trying to fix it

[ssproessig]

I'll have to get a GCC 4.9 first to fix it locally. I'll have a look at it in the evening.

[ssproessig]

I just disabled -Wuseless-cast, because I could not suppress it for all the tests (basically comes from Qt's moc), usually we use

inspired by: https://bugreports.qt.io/browse/QTBUG-82978

    # workaround moc's broken generated code
    if(${CMAKE_CXX_COMPILER_ID} STREQUAL GNU)
        set_source_files_properties("${TARGET}_autogen/mocs_compilation.cpp" PROPERTIES COMPILE_OPTIONS "-Wno-useless-cast")
    elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL Clang)
        set_source_files_properties("${TARGET}_autogen/mocs_compilation.cpp" PROPERTIES COMPILE_OPTIONS "-Wno-extra-semi-stmt;-Wno-redundant-parens")
    endif()

[ssproessig]

I also disabled -Wshadow for now, because I'm not aware of your naming conventions (e.g. using a / an prefix for arguments to methods) and didn't want to change to much of the code without clarification first.

[simonbrunel]

I also disabled -Wshadow for now, because I'm not aware of your naming conventions...

I was about to suggest to not include -Wshadow because of the argument name changes. I don't want to prefix all arguments with a or arg and I don't like to have just a few of them prefixed, making it inconsistent. I will take a look at how to rename each arg individually. Though, it's strange that only the error argument triggers an issue for you with Clang.

[simonbrunel]

Thanks @ssproessig