Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Enabling some commonly enabled compiler diagnostics #17344

Open
practicalswift opened this issue Nov 1, 2019 · 21 comments
Open

RFC: Enabling some commonly enabled compiler diagnostics #17344

practicalswift opened this issue Nov 1, 2019 · 21 comments

Comments

@practicalswift
Copy link
Member

@practicalswift practicalswift commented Nov 1, 2019

I've compiled a list of commonly enabled compiler diagnostics that we are currently choosing not to enable.

This is the list:

Compiler diagnostic no# of emitted unique GCC warnings in master no# of emitted unique Clang warnings in master
-Wconditional-uninitialized: Warn if a variable may be uninitialized when used Not supported 2
-Wdouble-promotion: Warn if float is implicit promoted to double 1 8
-Wduplicated-branches: Warn if if/else branches have duplicated code 0 Not supported
-Wduplicated-cond: Warn if if/else chain has duplicated conditions 0 Not supported
-Wfloat-equal: Warn if floating-point values are used in equality comparisons 29 18
-Wlogical-op: Warn about logical operations being used where bitwise were probably wanted 0 Not supported
-Wnon-virtual-dtor: Warn the user if a class with virtual functions has a non-virtual destructor. This helps catch hard to track down memory errors. 22 10
-Wnull-dereference: Warn if a potential nullptr dereference is detected 48 Not supported
-Woverloaded-virtual: Warn if you overload (not override) a virtual function 0 0
-Wsuggest-override: Warn about overriding virtual functions that are not marked with the override keyword 303 (of which 192 are in src/leveldb/ Not supported
-Wunreachable-code-loop-increment: Warn if a loop will run only once (loop increment never executed) Not supported 1
-Wunused-member-function: Warn on unused member function Not supported 2
-Wunused-template: Warn on unused template Not supported 1

There is a large overlap between this list and Jason Turner's list of recommended compiler diagnostics in the Collaborative Collection of C++ Best Practices (cppbestpractices) project. There is also an overlap with the recommendations given in the C++ Core Guidelines (with editors Bjarne Stroustrup and Herb Sutter).

I'm now seeking feedback regarding these diagnostics: which of these would make sense to enable in the Bitcoin Core project? :)

@hebasto

This comment has been minimized.

Copy link
Member

@hebasto hebasto commented Nov 2, 2019

Do numbers of warnings include warnings in subtrees (e.g., leveldb, secp256k1)?

@practicalswift

This comment has been minimized.

Copy link
Member Author

@practicalswift practicalswift commented Nov 2, 2019

@hebasto

Yes! :)

More specifically the warnings were recorded using:

$ git diff
diff --git a/configure.ac b/configure.ac
index 9f2942dc9..20ef11346 100644
--- a/configure.ac
+++ b/configure.ac
@@ -334,7 +334,7 @@ fi
 if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wall],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wall"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wextra],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wextra"],,[[$CXXFLAG_WERROR]])
-  AX_CHECK_COMPILE_FLAG([-Wformat],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wformat=2],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat=2"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wvla],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wswitch],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wswitch"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
@@ -342,6 +342,19 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wrange-loop-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wrange-loop-analysis"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wredundant-decls],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wredundant-decls"],,[[$CXXFLAG_WERROR]])

+  AX_CHECK_COMPILE_FLAG([-Wconditional-uninitialized],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wconditional-uninitialized"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wdouble-promotion],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdouble-promotion"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wduplicated-branches],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wduplicated-branches"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wduplicated-cond],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wduplicated-cond"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wfloat-equal],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wfloat-equal"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wlogical-op],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wlogical-op"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wnon-virtual-dtor],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wnon-virtual-dtor"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wnull-dereference],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wnull-dereference"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Woverloaded-virtual],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Woverloaded-virtual"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunreachable-code-loop-increment],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-loop-increment"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunused-member-function],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-member-function"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunused-template],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-template"],,[[$CXXFLAG_WERROR]])
+
   ## Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
   ## unknown options if any other warning is produced. Test the -Wfoo case, and
   ## set the -Wno-foo case if it works.
$ ./autogen.sh
$ CC=gcc CXX=g++ ./configure --with-incompatible-bdb
$ make 2>&1 > warnings-gcc.txt
$ make clean
$ CC=clang CXX=clang++ ./configure --with-incompatible-bdb
$ make 2>&1 > warnings-clang.txt
@laanwj

This comment has been minimized.

Copy link
Member

@laanwj laanwj commented Nov 2, 2019

Wformat=2: Warn on security issues around functions that format output (ie printf). We are currently using -Wformat=1 which doesn't enable -Wformat-security (part of -Wformat=2).

Seems useless as we disallow all printf family functions because of locale dependency.

@laanwj

This comment has been minimized.

Copy link
Member

@laanwj laanwj commented Nov 2, 2019

Also -Wsuggest-override would be kind of nice (#16710).

@practicalswift

This comment has been minimized.

Copy link
Member Author

@practicalswift practicalswift commented Nov 3, 2019

@laanwj

Seems useless as we disallow all printf family functions because of locale dependency.

I was hoping that we could apply __attribute__ ((format (printf, 1, 2))) style annotations to LogPrintf and our other functions of that style to get proper compile-time string formatting checking, but after some tinkering I'm afraid that route is not possible.

OTOH, no harm in bumping from -Wformat=1 to -Wformat=2 to get the stricter checking for the few printf-style standard functions we actually use (vsnprintf, fprintf, etc.).

Also -Wsuggest-override would be kind of nice (#16710).

Good point! I've added -Wsuggest-override to the list.

Unfortunately src/leveldb/ produces 192 (!) override warnings - so we'll have to suppress that :)

@hebasto

This comment has been minimized.

Copy link
Member

@hebasto hebasto commented Nov 3, 2019

@practicalswift

Unfortunately src/leveldb/ produces 192 (!) override warnings - so we'll have to suppress that :)

Even on top of #16722 ?
;)

@practicalswift

This comment has been minimized.

Copy link
Member Author

@practicalswift practicalswift commented Nov 3, 2019

@hebasto

No #16722 would solve that. I hope it gets merged soon :)

@laanwj

This comment has been minimized.

Copy link
Member

@laanwj laanwj commented Nov 4, 2019

I was hoping that we could apply attribute ((format (printf, 1, 2))) style annotations to LogPrintf and our other functions of that style to get proper compile-time string formatting checking, but after some tinkering I'm afraid that route is not possible.

It's indeed not possible. But also this would be undesirable as that checks a few things that are irrelevant in tinyformat due to type-safety:

  • size modifiers don't matter (llx, …), they would only create noise
  • you can use %s in place of everything
  • std::string can (and should) be passed in as-is
  • dangerous format characters such as %n doesn't exist

We also already have test/lint/lint-format-strings.py to do custom checks attuned to our way of formatting. It doesn't seem to happen often but this is an old, solved problem.

@practicalswift

This comment has been minimized.

Copy link
Member Author

@practicalswift practicalswift commented Nov 4, 2019

@laanwj Good points. I've now removed -Wformat=2 from the list.

Can we come up with any good arguments against any of the remaining diagnostics in the list or do they all seem reasonable?

@MarcoFalke

This comment has been minimized.

Copy link
Member

@MarcoFalke MarcoFalke commented Nov 5, 2019

@Sjors

This comment has been minimized.

Copy link
Member

@Sjors Sjors commented Nov 5, 2019

Concept ACK on having one or more Travis instances check for (some / most of) these issues, as well as maybe turning them on by default. We'll have to try that PR on various developer systems, with and without depends, to see if it doesn't cause problems. For example I tend to get a flood of spurious warnings on macOS Catalina, which cause me to miss relevant warnings. I'd have to check which warning types occur in the spurious ones.

@practicalswift

This comment has been minimized.

Copy link
Member Author

@practicalswift practicalswift commented Nov 5, 2019

@MarcoFalke @Sjors The scope of this issue is enabling currently disabled diagnostics. -Wunused-variable is already enabled in both Clang and GCC by our use of -Wall.

With that said: @Sjors - if you want to implement #17377 (comment) I would support such PR :)

@laanwj

This comment has been minimized.

Copy link
Member

@laanwj laanwj commented Nov 5, 2019

I'm generally okay with adding additional diagnostics, especially those covered by both gcc and clang, and which don't diverge too much between compiler versions. I definitely don't want to repeat the debacle around variable shadowing checks.

@Sjors

This comment has been minimized.

Copy link
Member

@Sjors Sjors commented Nov 6, 2019

Ah yes, I was talking about -Werror=unused-variable and such. But having warnings locally by default would be good too, except when it becomes a flood of existing issues .

@laanwj

This comment has been minimized.

Copy link
Member

@laanwj laanwj commented Nov 6, 2019

@practicalswift maybe upload the full list of warnings somewhere for all these diagnostics enabled; then we have a better grasp on how useful, or spammy, these particular reports are

MarcoFalke added a commit that referenced this issue Nov 15, 2019
18b18f8 [build] ./configure --enable-werror: add unused-variable (Sjors Provoost)

Pull request description:

  The two macOS Travis machines run with `--enable-werror`. This PR adds `-Werror=unused-variable` to the existing `vla`, `switch` and `thread-safety-analysis` checks. This should prevent the need for fixes like b07b07c, 26a93bc, dd777f3, 99be644, fa39f67, 16bcc1b, bb079a0, bdaed47 and ecf9b25 with minimal nuisance.

  Thoughts for followups:
  * Travis starts these macOS machines fairly late, so we should consider setting `--enable-werror` on earlier machines as well.
  * We should encourage the use of `--enable-werror` by developers. Maybe switch it on by default for `--enable-debug`?
  * See practicalswift's overview of other checks to consider in #17344

ACKs for top commit:
  MarcoFalke:
    ACK 18b18f8
  practicalswift:
    ACK 18b18f8 -- nice!

Tree-SHA512: 892b471ca5ea547f3c952ac88190cbebf8110cb7aec6f20466aeb312aeb0910bfe990f914e153c40ecb55709c03775ef30770412ad76f9d532ca77055596c582
sidhujag added a commit to syscoin/syscoin that referenced this issue Nov 15, 2019
18b18f8 [build] ./configure --enable-werror: add unused-variable (Sjors Provoost)

Pull request description:

  The two macOS Travis machines run with `--enable-werror`. This PR adds `-Werror=unused-variable` to the existing `vla`, `switch` and `thread-safety-analysis` checks. This should prevent the need for fixes like b07b07c, 26a93bc, dd777f3, 99be644, fa39f67, 16bcc1b, bb079a0, bdaed47 and ecf9b25 with minimal nuisance.

  Thoughts for followups:
  * Travis starts these macOS machines fairly late, so we should consider setting `--enable-werror` on earlier machines as well.
  * We should encourage the use of `--enable-werror` by developers. Maybe switch it on by default for `--enable-debug`?
  * See practicalswift's overview of other checks to consider in bitcoin#17344

ACKs for top commit:
  MarcoFalke:
    ACK 18b18f8
  practicalswift:
    ACK 18b18f8 -- nice!

Tree-SHA512: 892b471ca5ea547f3c952ac88190cbebf8110cb7aec6f20466aeb312aeb0910bfe990f914e153c40ecb55709c03775ef30770412ad76f9d532ca77055596c582
@practicalswift

This comment has been minimized.

Copy link
Member Author

@practicalswift practicalswift commented Dec 12, 2019

Some additional diagnostics to consider and their current warning counts. Tested under Clang 10.

-Wdeprecated-copy-dtor (5 instances)
-Wdisabled-macro-expansion (1 instance)
-Wimplicit-fallthrough (9 instances)
-Winconsistent-missing-destructor-override (1 instance)
-Wshadow (note: might be controversial given memories of historical compiler issues with
                this warning type, 5 instances)
-Wshadow-uncaptured-local (2 instances)
-Wtautological-unsigned-enum-zero-compare (3 instances)
-Wthread-safety-attributes (1 instance)
-Wunreachable-code-break (10 instances)
-Wunreachable-code-return (1 instance)
@hebasto

This comment has been minimized.

Copy link
Member

@hebasto hebasto commented Dec 12, 2019

@practicalswift

Tested under Clang 10.

Some Apple Clang 10 warnings are removed in #16641.

@practicalswift

This comment has been minimized.

Copy link
Member Author

@practicalswift practicalswift commented Jan 8, 2020

I'm considering upstreaming a patch I'm using locally. The patch enables a subset of the Clang warnings suggested in this issue.

The warnings enabled are such that they a.) can help avoid real bugs, b.) can be enabled without false positives and c.) do not introduce a massive number of warnings when compiling against current master.

These are the enabled diagnostics:

  • -Wdouble-promotion: Warn if float is implicit promoted to double.
  • -Wimplicit-fallthrough: Warn on unannotated fall-through between switch labels.
  • -Woverloaded-virtual: Warn if you overload (not override) a virtual function.
  • -Wshadow-uncaptured-local: Warn if a declaration shadows a variable.
  • -Wunreachable-code-loop-increment: Warn if a loop will run only once (loop increment never executed).
  • -Wunreachable-code-return: Warn if a return statement will never be executed.
  • -Wunused-member-function: Warn on unused member function.
  • -Wunused-template: Warn on unused template.

Note that I'm not suggesting -Werror for any of these :)

Any feedback? Should any of the suggested checks be removed before being submitted as a PR?

I'll leave this up for discussion and submit a PR next week based on the feedback in this issue :)

This is the diff if you want to try it out:

diff --git a/configure.ac b/configure.ac
index 888f67cc8..dc13955f0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -344,6 +344,14 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wredundant-decls],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wredundant-decls"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wunused-variable],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-variable"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wdate-time],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdate-time"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wdouble-promotion],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdouble-promotion"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wimplicit-fallthrough"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Woverloaded-virtual],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Woverloaded-virtual"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wshadow-uncaptured-local],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wshadow-uncaptured-local"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunreachable-code-loop-increment],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-loop-increment"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunreachable-code-return],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-return"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunused-member-function],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-member-function"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunused-template],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-template"],,[[$CXXFLAG_WERROR]])

   dnl Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
   dnl unknown options if any other warning is produced. Test the -Wfoo case, and
@fanquake

This comment has been minimized.

Copy link
Member

@fanquake fanquake commented Jan 10, 2020

Any feedback? Should any of the suggested checks be removed before being submitted as a PR?

-Wimplicit-fallthrough:

We explicitly disable this (just below), so having it in this diff doesn't enable anything. I also don't think we'd turn it on, given that the original reasons it was disabled still apply (large number of warnings in Boost, leveldb and tinyformat etc, see #10489). This is also part of -Wextra for GCC.

-Wunused-template

This introduces a large number of Qt related warnings for me:

  CXX      qt/libbitcoinqt_a-platformstyle.o
In file included from qt/test/wallettests.cpp:1:
In file included from ./qt/test/wallettests.h:5:
In file included from /usr/local/Cellar/qt/5.14.0/lib/QtTest.framework/Headers/QTest:1:
In file included from /usr/local/Cellar/qt/5.14.0/lib/QtTest.framework/Headers/qtest.h:449:
In file included from /usr/local/Cellar/qt/5.14.0/lib/QtTest.framework/Headers/qtestsystem.h:45:
/usr/local/Cellar/qt/5.14.0/lib/QtCore.framework/Headers/qtestsupport_core.h:55:31: warning: unused function template 'qWaitFor' [-Wunused-template]
Q_REQUIRED_RESULT static bool qWaitFor(Functor predicate, int timeout = 5000)
                              ^
1 warning generated.

For everything below, ACK opening a PR, assuming you are fixing any warnings where applicable, and/or have submitted PRs upstream. Note however that this was only doing a quick check on macOS, with brew installed dependencies, so the output could be much worse on other platforms. I also did a quick check on Ubuntu Bionic and saw similar results.

-Wdouble-promotion

torcontrol.cpp:702:51: warning: implicit conversion increases floating-point precision: 'float' to 'double' [-Wdouble-promotion]
    struct timeval time = MillisToTimeval(int64_t(reconnect_timeout * 1000.0));
                                                  ^~~~~~~~~~~~~~~~~ ~
1 warning generated.

qt/splashscreen.cpp:54:32: warning: implicit conversion increases floating-point precision: 'float' to 'qreal' (aka 'double') [-Wdouble-promotion]
    pixmap.setDevicePixelRatio(devicePixelRatio);
           ~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~
qt/splashscreen.cpp:60:61: warning: implicit conversion increases floating-point precision: 'float' to 'qreal' (aka 'double') [-Wdouble-promotion]
    QRadialGradient gradient(QPoint(0,0), splashSize.width()/devicePixelRatio);
                    ~~~~~~~~              ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
2 warnings generated.

qt/trafficgraphwidget.cpp:86:94: warning: implicit conversion increases floating-point precision: 'float' to 'double' [-Wdouble-promotion]
    painter.drawText(XMARGIN, YMARGIN + h - h * val / fMax-yMarginText, QString("%1 %2").arg(val).arg(units));
                                                                                         ~~~ ^~~
qt/trafficgraphwidget.cpp:96:98: warning: implicit conversion increases floating-point precision: 'float' to 'double' [-Wdouble-promotion]
        painter.drawText(XMARGIN, YMARGIN + h - h * val / fMax-yMarginText, QString("%1 %2").arg(val).arg(units));
                                                                                             ~~~ ^~~
2 warnings generated.

-Wunreachable-code-loop-increment

init.cpp:921:5: warning: loop will run at most once (loop increment never executed) [-Wunreachable-code-loop-increment]
    for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) {
    ^~~
1 warning generated.

-Wunused-member-function

script/bitcoinconsensus.cpp:50:9: warning: unused member function 'GetType' [-Wunused-member-function]
    int GetType() const { return m_type; }
        ^
1 warning generated.

index/blockfilterindex.cpp:55:5: warning: unused member function 'DBHeightKey' [-Wunused-member-function]
    DBHeightKey() : height(0) {}
    ^
index/blockfilterindex.cpp:81:5: warning: unused function template 'Unserialize' [-Wunused-template]
    ADD_SERIALIZE_METHODS;
    ^
./serialize.h:198:10: note: expanded from macro 'ADD_SERIALIZE_METHODS'
    void Unserialize(Stream& s) {                                     \
         ^
2 warnings generated.

test/util_tests.cpp:1919:14: warning: unused member function 'operator=' [-Wunused-member-function]
    Tracker& operator=(Tracker&& t) noexcept
             ^
1 warning generated.

-Wunreachable-code-return

leveldb/db/log_reader.cc:181:10: warning: 'return' will never be executed [-Wunreachable-code-return]
  return false;
         ^~~~~
1 warning generated.
@practicalswift

This comment has been minimized.

Copy link
Member Author

@practicalswift practicalswift commented Jan 10, 2020

@fanquake

Thanks a lot for taking the time to test! That is appreciated!

-Wimplicit-fallthrough:

We explicitly disable this (just below), so having it in this diff doesn't enable anything. I also don't think we'd turn it on, given that the original reasons it was disabled still apply (large number of warnings in Boost, leveldb and tinyformat etc, see #10489). This is also part of -Wextra for GCC.

Oh, good catch! Now removed.

-Wunused-template

This introduces a large number of Qt related warnings for me:

Removed.

Updated diff if someone wants to try it out:

diff --git a/configure.ac b/configure.ac
index 888f67cc8..e4a7423bd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -344,6 +344,12 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wredundant-decls],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wredundant-decls"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wunused-variable],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-variable"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wdate-time],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdate-time"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wdouble-promotion],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdouble-promotion"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Woverloaded-virtual],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Woverloaded-virtual"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wshadow-uncaptured-local],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wshadow-uncaptured-local"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunreachable-code-loop-increment],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-loop-increment"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunreachable-code-return],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-return"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wunused-member-function],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-member-function"],,[[$CXXFLAG_WERROR]])

   dnl Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
   dnl unknown options if any other warning is produced. Test the -Wfoo case, and
@fanquake

This comment has been minimized.

Copy link
Member

@fanquake fanquake commented Jan 20, 2020

@practicalswift Are you still planning on opening a PR with any of these changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.