build: Enable some commonly enabled compiler diagnostics #19015

practicalswift · 2020-05-19T12:39:54Z

Enable some commonly enabled compiler diagnostics as discussed in #17344.

Compiler diagnostic	no# of emitted unique GCC warnings in `master`	no# of emitted unique Clang warnings in `master`
`-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
`-Wlogical-op`: Warn about logical operations being used where bitwise were probably wanted	0	Not supported
`-Woverloaded-virtual`: Warn if you overload (not `override`) a virtual function	0	0
~~`-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).

Closes #17344.

maflcko · 2020-05-19T13:10:29Z

Concept ACK, but before merge we should check that there are no warnings. This includes warnings in headers of third-party libraries. Boost and Qt have a good track record of producing warnings.

hebasto · 2020-05-19T16:18:25Z

Concept ACK.

DrahtBot · 2020-05-22T01:34:53Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: avoid repetitions when enabling warnings in configure.ac #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Enable unreachable-code-loop-increment and treat as error. refs: bitcoin#19015

eea8114 build: Enable unreachable-code-loop-increment (Jonathan Schoeller) d15db4b refactor: Fix unreachable code in init arg checks (Jonathan Schoeller) Pull request description: Closes: #19017 In #19015 it's been suggested that we add some new compiler warnings to our build. Some of these, such as `-Wunreachable-code-loop-increment`, generate warnings. We'll likely want to fix these up if we're going to turn these warnings on. ```shell init.cpp:969: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. ``` https://github.com/bitcoin/bitcoin/blob/aa8d76806c74a51ec66e5004394fe9ea8ff0fac4/src/init.cpp#L968-L972 To fix this, collect all errors, and output them in a single error message after the loop completes. This resolves the unreachable code warning, and avoids popup hell that could result from outputting a seperate message for each error or warning one by one. ACKs for top commit: laanwj: Code review ACK eea8114 hebasto: re-ACK eea8114, only suggested changes applied since the [previous](#19131 (review)) review. Tree-SHA512: 2aa3ceb7fab581b6ba2580900668388d8eba1c3001c8ff9c11c1f4a9a10fbc37f30e590249862676858446e3f4950140a252953ba1643ba3bfd772f8eae20583

hebasto · 2020-06-04T16:03:01Z

@practicalswift Why this PR does not include changes to get rid of new warnings?

master (39afe5b) + this PR:

$ make -j 9 > /dev/null 
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:54:5: warning: unused member function 'DBHeightKey' [-Wunused-member-function]
    DBHeightKey() : height(0) {}
    ^
index/blockfilterindex.cpp:80:5: warning: unused function template 'Unserialize' [-Wunused-template]
    SERIALIZE_METHODS(DBHashKey, obj) {
    ^
./serialize.h:215:10: note: expanded from macro 'SERIALIZE_METHODS'
    void Unserialize(Stream& s)                                                     \
         ^
index/blockfilterindex.cpp:80:5: warning: unused function template 'Unser' [-Wunused-template]
./serialize.h:220:5: note: expanded from macro 'SERIALIZE_METHODS'
    FORMATTER_METHODS(cls, obj)
    ^
./serialize.h:196:17: note: expanded from macro 'FORMATTER_METHODS'
    static void Unser(Stream& s, cls& obj) { SerializationOps(obj, s, CSerActionUnserialize()); } \
                ^
3 warnings generated.
script/bitcoinconsensus.cpp:50:9: warning: unused member function 'GetType' [-Wunused-member-function]
    int GetType() const { return m_type; }
        ^
1 warning generated.
test/util_tests.cpp:1972:14: warning: member function 'operator=' is not needed and will not be emitted [-Wunneeded-member-function]
    Tracker& operator=(Tracker&& t) noexcept
             ^
1 warning generated.

eea8114 build: Enable unreachable-code-loop-increment (Jonathan Schoeller) d15db4b refactor: Fix unreachable code in init arg checks (Jonathan Schoeller) Pull request description: Closes: bitcoin#19017 In bitcoin#19015 it's been suggested that we add some new compiler warnings to our build. Some of these, such as `-Wunreachable-code-loop-increment`, generate warnings. We'll likely want to fix these up if we're going to turn these warnings on. ```shell init.cpp:969: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. ``` https://github.com/bitcoin/bitcoin/blob/aa8d76806c74a51ec66e5004394fe9ea8ff0fac4/src/init.cpp#L968-L972 To fix this, collect all errors, and output them in a single error message after the loop completes. This resolves the unreachable code warning, and avoids popup hell that could result from outputting a seperate message for each error or warning one by one. ACKs for top commit: laanwj: Code review ACK eea8114 hebasto: re-ACK eea8114, only suggested changes applied since the [previous](bitcoin#19131 (review)) review. Tree-SHA512: 2aa3ceb7fab581b6ba2580900668388d8eba1c3001c8ff9c11c1f4a9a10fbc37f30e590249862676858446e3f4950140a252953ba1643ba3bfd772f8eae20583

practicalswift · 2020-06-05T07:35:09Z

@hebasto Thanks for testing! I wanted to keep this PR build-system only to keep review simple. Feel free to tackle the warnings in a separate PR if you want :)

practicalswift · 2020-06-07T10:49:40Z

@MarcoFalke I don't think this one needs rebase - anything else that you had in mind with the "waiting for author" label? :)

maflcko · 2020-06-07T10:55:11Z

-Wunreachable-code-loop-increment has been merged (silent merge conflict), so it needs to be removed (with or without a rebase)

practicalswift · 2020-06-07T19:40:46Z

@MarcoFalke Oh, thanks for clarifying! Now fixed :)

practicalswift · 2020-06-08T14:44:27Z

The Travis TSan job failure is unrelated but I'm unable to re-run the job :)

maflcko · 2020-06-08T14:53:44Z

Another rebase would make it rerun

Enable unreachable-code-loop-increment and treat as error. refs: bitcoin#19015

fanquake · 2020-08-10T11:17:29Z

Concept ACK

Thanks for testing! I wanted to keep this PR build-system only to keep review simple. Feel free to tackle the warnings in a separate PR if you want :)

There's not really a rush to do this, and it's unlikely this will be merged while it introduces any new (presumably easy to fix?) warnings. So if we're going to turn new diagnostics on, let's just take care of any warnings now, or, if you'd rather not deal with code changes, please reduce the scope of this PR.

I checked out this branch and started building. Spamming the log with the following is not going to be useful, and in this case it's coming from the brew installed Boost:

Options used to compile and link:
  multiprocess  = no
  with wallet   = yes
  with gui / qt = yes
    with qr     = yes
  with zmq      = yes
  with test     = yes
    with fuzz   = no
  with bench    = yes
  with upnp     = yes
  use asm       = yes
  sanitizers    = 
  debug enabled = no
  gprof enabled = no
  werror        = no

  target os     = darwin
  build os      = darwin19.6.0

  CC            = /usr/local/bin/ccache gcc
  CFLAGS        = -g -O2
  CPPFLAGS      =   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -I/usr/local/opt/berkeley-db@4/include -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0
  CXX           = /usr/local/bin/ccache g++ -std=c++11
  CXXFLAGS      =   -Wstack-protector -fstack-protector-all  -Wall -Wextra -Wgnu -Wformat -Wvla -Wshadow-field -Wswitch -Wformat-security -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunused-variable -Wdate-time -Wconditional-uninitialized -Wsign-compare -Woverloaded-virtual -Wunused-member-function -Wunused-template -Wunreachable-code-loop-increment  -Wno-unused-parameter -Wno-self-assign -Wno-unused-local-typedef -Wno-deprecated-register -Wno-implicit-fallthrough   -g -O2
  LDFLAGS       = -pthread  -Wl,-bind_at_load   -Wl,-headerpad_max_install_names -Wl,-dead_strip -Wl,-dead_strip_dylibs
  ARFLAGS       = cr

/Applications/Xcode.app/Contents/Developer/usr/bin/make -C src bitcoind
  CXX      bitcoind-bitcoind.o
  CXX      libbitcoin_server_a-addrdb.o
  CXX      libbitcoin_server_a-addrman.o
  CXX      libbitcoin_server_a-banman.o
  CXX      libbitcoin_server_a-blockencodings.o
  CXX      libbitcoin_server_a-blockfilter.o
  CXX      libbitcoin_server_a-chain.o
  CXX      libbitcoin_server_a-flatfile.o
  CXX      libbitcoin_server_a-httprpc.o
  CXX      libbitcoin_server_a-httpserver.o
  CXX      libbitcoin_server_a-init.o
  CXX      libbitcoin_server_a-dbwrapper.o
  CXX      libbitcoin_server_a-miner.o
  CXX      libbitcoin_server_a-net.o
  CXX      libbitcoin_server_a-net_processing.o
In file included from blockencodings.cpp:13:
In file included from ./txmempool.h:26:
In file included from /usr/local/include/boost/multi_index_container.hpp:32:
In file included from /usr/local/include/boost/multi_index_container_fwd.hpp:19:
In file included from /usr/local/include/boost/multi_index/ordered_index_fwd.hpp:16:
In file included from /usr/local/include/boost/multi_index/detail/ord_index_args.hpp:21:
In file included from /usr/local/include/boost/multi_index/tag.hpp:17:
In file included from /usr/local/include/boost/multi_index/detail/no_duplicate_tags.hpp:18:
In file included from /usr/local/include/boost/mpl/set/set0.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/at_impl.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/has_key_impl.hpp:21:
In file included from /usr/local/include/boost/mpl/aux_/overload_names.hpp:17:
/usr/local/include/boost/mpl/aux_/ptr_to_ref.hpp:42:40: warning: unused function template 'ptr_to_ref' [-Wunused-template]
template< typename T > static T const& ptr_to_ref(T*);
                                       ^
1 warning generated.
  CXX      libbitcoin_server_a-noui.o
  CXX      libbitcoin_server_a-pow.o
  CXX      libbitcoin_server_a-rest.o
  CXX      libbitcoin_server_a-shutdown.o
  CXX      libbitcoin_server_a-timedata.o
  CXX      libbitcoin_server_a-torcontrol.o
  CXX      libbitcoin_server_a-txdb.o
In file included from miner.cpp:6:
In file included from ./miner.h:11:
In file included from ./txmempool.h:26:
In file included from /usr/local/include/boost/multi_index_container.hpp:32:
In file included from /usr/local/include/boost/multi_index_container_fwd.hpp:19:
In file included from /usr/local/include/boost/multi_index/ordered_index_fwd.hpp:16:
In file included from /usr/local/include/boost/multi_index/detail/ord_index_args.hpp:21:
In file included from /usr/local/include/boost/multi_index/tag.hpp:17:
In file included from /usr/local/include/boost/multi_index/detail/no_duplicate_tags.hpp:18:
In file included from /usr/local/include/boost/mpl/set/set0.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/at_impl.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/has_key_impl.hpp:21:
In file included from /usr/local/include/boost/mpl/aux_/overload_names.hpp:17:
/usr/local/include/boost/mpl/aux_/ptr_to_ref.hpp:42:40: warning: unused function template 'ptr_to_ref' [-Wunused-template]
template< typename T > static T const& ptr_to_ref(T*);
                                       ^
1 warning generated.
  CXX      libbitcoin_server_a-txmempool.o
  CXX      libbitcoin_server_a-ui_interface.o
  CXX      libbitcoin_server_a-validation.o
In file included from rest.cpp:19:
In file included from ./txmempool.h:26:
In file included from /usr/local/include/boost/multi_index_container.hpp:32:
In file included from /usr/local/include/boost/multi_index_container_fwd.hpp:19:
In file included from /usr/local/include/boost/multi_index/ordered_index_fwd.hpp:16:
In file included from /usr/local/include/boost/multi_index/detail/ord_index_args.hpp:21:
In file included from /usr/local/include/boost/multi_index/tag.hpp:17:
In file included from /usr/local/include/boost/multi_index/detail/no_duplicate_tags.hpp:18:
In file included from /usr/local/include/boost/mpl/set/set0.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/at_impl.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/has_key_impl.hpp:21:
In file included from /usr/local/include/boost/mpl/aux_/overload_names.hpp:17:
/usr/local/include/boost/mpl/aux_/ptr_to_ref.hpp:42:40: warning: unused function template 'ptr_to_ref' [-Wunused-template]
template< typename T > static T const& ptr_to_ref(T*);
                                       ^
1 warning generated.
  CXX      libbitcoin_server_a-validationinterface.o
  CXX      libbitcoin_server_a-versionbits.o
  CXX      interfaces/libbitcoin_wallet_a-wallet.o
  CXX      wallet/libbitcoin_wallet_a-coincontrol.o
In file included from txmempool.cpp:6:
In file included from ./txmempool.h:26:
In file included from /usr/local/include/boost/multi_index_container.hpp:32:
In file included from /usr/local/include/boost/multi_index_container_fwd.hpp:19:
In file included from /usr/local/include/boost/multi_index/ordered_index_fwd.hpp:16:
In file included from /usr/local/include/boost/multi_index/detail/ord_index_args.hpp:21:
In file included from /usr/local/include/boost/multi_index/tag.hpp:17:
In file included from /usr/local/include/boost/multi_index/detail/no_duplicate_tags.hpp:18:
In file included from /usr/local/include/boost/mpl/set/set0.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/at_impl.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/has_key_impl.hpp:21:
In file included from /usr/local/include/boost/mpl/aux_/overload_names.hpp:17:
/usr/local/include/boost/mpl/aux_/ptr_to_ref.hpp:42:40: warning: unused function template 'ptr_to_ref' [-Wunused-template]
template< typename T > static T const& ptr_to_ref(T*);
                                       ^
1 warning generated.
  CXX      wallet/libbitcoin_wallet_a-context.o
  CXX      wallet/libbitcoin_wallet_a-crypter.o
  CXX      wallet/libbitcoin_wallet_a-db.o
In file included from init.cpp:28:
In file included from ./miner.h:11:
In file included from ./txmempool.h:26:
In file included from /usr/local/include/boost/multi_index_container.hpp:32:
In file included from /usr/local/include/boost/multi_index_container_fwd.hpp:19:
In file included from /usr/local/include/boost/multi_index/ordered_index_fwd.hpp:16:
In file included from /usr/local/include/boost/multi_index/detail/ord_index_args.hpp:21:
In file included from /usr/local/include/boost/multi_index/tag.hpp:17:
In file included from /usr/local/include/boost/multi_index/detail/no_duplicate_tags.hpp:18:
In file included from /usr/local/include/boost/mpl/set/set0.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/at_impl.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/has_key_impl.hpp:21:
In file included from /usr/local/include/boost/mpl/aux_/overload_names.hpp:17:
/usr/local/include/boost/mpl/aux_/ptr_to_ref.hpp:42:40: warning: unused function template 'ptr_to_ref' [-Wunused-template]
template< typename T > static T const& ptr_to_ref(T*);
                                       ^
1 warning generated.
  CXX      wallet/libbitcoin_wallet_a-feebumper.o
  CXX      wallet/libbitcoin_wallet_a-fees.o
  CXX      wallet/libbitcoin_wallet_a-load.o
  CXX      wallet/libbitcoin_wallet_a-rpcdump.o
In file included from net_processing.cpp:16:
In file included from ./validation.h:22:
In file included from ./txmempool.h:26:
In file included from /usr/local/include/boost/multi_index_container.hpp:32:
In file included from /usr/local/include/boost/multi_index_container_fwd.hpp:19:
In file included from /usr/local/include/boost/multi_index/ordered_index_fwd.hpp:16:
In file included from /usr/local/include/boost/multi_index/detail/ord_index_args.hpp:21:
In file included from /usr/local/include/boost/multi_index/tag.hpp:17:
In file included from /usr/local/include/boost/multi_index/detail/no_duplicate_tags.hpp:18:
In file included from /usr/local/include/boost/mpl/set/set0.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/at_impl.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/has_key_impl.hpp:21:
In file included from /usr/local/include/boost/mpl/aux_/overload_names.hpp:17:
/usr/local/include/boost/mpl/aux_/ptr_to_ref.hpp:42:40: warning: unused function template 'ptr_to_ref' [-Wunused-template]
template< typename T > static T const& ptr_to_ref(T*);
                                       ^
1 warning generated.

> Killed build.

practicalswift · 2020-08-11T09:05:59Z

@fanquake Thanks for testing, and your feedback makes sense: I'll adjust accordingly. I don't have any macOS machine to test on -- would you mind sharing the results of make 2>&1 | grep "warning: " | sort | uniq -c? :)

fanquake · 2020-08-11T11:52:53Z

I don't have any macOS machine to test on

You should just be able to just check the Travis output:

Making install in src
< snip >
  CXX      libbitcoin_server_a-init.o
In file included from blockencodings.cpp:13:
In file included from ./txmempool.h:26:
In file included from /usr/local/include/boost/multi_index_container.hpp:32:
In file included from /usr/local/include/boost/multi_index_container_fwd.hpp:19:
In file included from /usr/local/include/boost/multi_index/ordered_index_fwd.hpp:16:
In file included from /usr/local/include/boost/multi_index/detail/ord_index_args.hpp:21:
In file included from /usr/local/include/boost/multi_index/tag.hpp:17:
In file included from /usr/local/include/boost/multi_index/detail/no_duplicate_tags.hpp:18:
In file included from /usr/local/include/boost/mpl/set/set0.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/at_impl.hpp:18:
In file included from /usr/local/include/boost/mpl/set/aux_/has_key_impl.hpp:21:
In file included from /usr/local/include/boost/mpl/aux_/overload_names.hpp:17:
/usr/local/include/boost/mpl/aux_/ptr_to_ref.hpp:42:40: warning: unused function template 'ptr_to_ref' [-Wunused-template]
template< typename T > static T const& ptr_to_ref(T*);
                                       ^
1 warning generated.

make 2>&1 | grep "warning: " | sort | uniq -c

  12 /usr/local/Cellar/qt/5.15.0/lib/QtCore.framework/Headers/qtestsupport_core.h:55:31: warning: unused function template 'qWaitFor' [-Wunused-template]
 137 /usr/local/include/boost/mpl/aux_/ptr_to_ref.hpp:42:40: warning: unused function template 'ptr_to_ref' [-Wunused-template]
   1 index/blockfilterindex.cpp:54:5: warning: unused member function 'DBHeightKey' [-Wunused-member-function]
   1 index/blockfilterindex.cpp:80:5: warning: unused function template 'Unser' [-Wunused-template]
   1 index/blockfilterindex.cpp:80:5: warning: unused function template 'Unserialize' [-Wunused-template]
   1 qt/bitcoin.cpp:259:53: warning: 'QFlags' is deprecated: Use default constructor instead [-Wdeprecated-declarations]
   1 qt/bitcoingui.cpp:1304:29: warning: 'pixmap' is deprecated: Use the other overload which returns QPixmap by-value [-Wdeprecated-declarations]
   1 qt/optionsmodel.cpp:222:59: warning: 'split' is deprecated: Use Qt::SplitBehavior variant instead [-Wdeprecated-declarations]
   1 qt/qrimagewidget.cpp:100:12: warning: 'pixmap' is deprecated: Use the other overload which returns QPixmap by-value [-Wdeprecated-declarations]
   1 qt/qrimagewidget.cpp:105:45: warning: 'pixmap' is deprecated: Use the other overload which returns QPixmap by-value [-Wdeprecated-declarations]
   1 qt/qrimagewidget.cpp:121:9: warning: 'pixmap' is deprecated: Use the other overload which returns QPixmap by-value [-Wdeprecated-declarations]
   1 qt/qrimagewidget.cpp:132:9: warning: 'pixmap' is deprecated: Use the other overload which returns QPixmap by-value [-Wdeprecated-declarations]
   1 qt/qrimagewidget.cpp:139:9: warning: 'pixmap' is deprecated: Use the other overload which returns QPixmap by-value [-Wdeprecated-declarations]
   1 qt/qrimagewidget.cpp:98:9: warning: 'pixmap' is deprecated: Use the other overload which returns QPixmap by-value [-Wdeprecated-declarations]
   1 qt/sendcoinsdialog.cpp:174:72: warning: 'buttonClicked' is deprecated: Use QButtonGroup::idClicked(int) instead [-Wdeprecated-declarations]
   1 qt/sendcoinsdialog.cpp:175:72: warning: 'buttonClicked' is deprecated: Use QButtonGroup::idClicked(int) instead [-Wdeprecated-declarations]
   1 qt/transactionview.cpp:240:85: warning: 'split' is deprecated: Use Qt::SplitBehavior variant instead [-Wdeprecated-declarations]
   1 qt/transactionview.cpp:278:17: warning: 'QDateTime' is deprecated: Use QDate::startOfDay() [-Wdeprecated-declarations]
   1 qt/transactionview.cpp:285:17: warning: 'QDateTime' is deprecated: Use QDate::startOfDay() [-Wdeprecated-declarations]
   1 qt/transactionview.cpp:291:17: warning: 'QDateTime' is deprecated: Use QDate::startOfDay() [-Wdeprecated-declarations]
   1 qt/transactionview.cpp:296:17: warning: 'QDateTime' is deprecated: Use QDate::startOfDay() [-Wdeprecated-declarations]
   1 qt/transactionview.cpp:297:17: warning: 'QDateTime' is deprecated: Use QDate::startOfDay() [-Wdeprecated-declarations]
   1 qt/transactionview.cpp:301:17: warning: 'QDateTime' is deprecated: Use QDate::startOfDay() [-Wdeprecated-declarations]
   1 qt/transactionview.cpp:586:13: warning: 'QDateTime' is deprecated: Use QDate::startOfDay() [-Wdeprecated-declarations]
   1 qt/transactionview.cpp:587:13: warning: 'QDateTime' is deprecated: Use QDate::startOfDay() [-Wdeprecated-declarations]
   1 random.cpp:257:13: warning: unused function 'GetDevURandom' [-Wunused-function]
   2 script/bitcoinconsensus.cpp:50:9: warning: unused member function 'GetType' [-Wunused-member-function]
   1 test/util_tests.cpp:1972:14: warning: unused member function 'operator=' [-Wunused-member-function]

practicalswift · 2020-08-11T12:10:57Z

I don't have any macOS machine to test on

You should just be able to just check the Travis output:

Oh, of course! Thanks! :)

I've now updated the PR to only enable -Wduplicated-branches, -Wduplicated-cond, -Wlogical-op and -Woverloaded-virtual. None of these should warn on current master.

Should be ready for final review :)

hebasto

ACK 2f8a4c9, no new warnings in Travis jobs.

Are there any reasons for not adding corresponding -Werror?

jonatack

ACK 2f8a4c9 no warnings for me with these locally on debian 5.7.10-1 (2020-07-26) x86_64 with gcc 10 and clang 12

fanquake

ACK 2f8a4c9 - no-longer seeing any obvious issues with doing this.

…gnostics 2f8a4c9 build: Enable some commonly enabled compiler diagnostics (practicalswift) Pull request description: Enable some commonly enabled compiler diagnostics as discussed in bitcoin#17344. | Compiler diagnostic | no# of emitted unique GCC warnings in `master` | no# of emitted unique Clang warnings in `master` | | ------------- | ------------- | ------------- | | `-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 | | `-Wlogical-op`: Warn about logical operations being used where bitwise were probably wanted | 0 | Not supported | | `-Woverloaded-virtual`: Warn if you overload (not `override`) a virtual function | 0 | 0 | | ~~`-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](https://github.com/lefticus/cppbestpractices/blob/master/02-Use_the_Tools_Available.md#gcc--clang). There is also an overlap with the recommendations given in the [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines) (with editors Bjarne Stroustrup and Herb Sutter). Closes bitcoin#17344. ACKs for top commit: jonatack: ACK 2f8a4c9 no warnings for me with these locally on debian 5.7.10-1 (2020-07-26) x86_64 with gcc 10 and clang 12 fanquake: ACK 2f8a4c9 - no-longer seeing any obvious issues with doing this. hebasto: ACK 2f8a4c9, no new warnings in Travis jobs. Tree-SHA512: f669ea22b31263a555f999eff6a9d65750662e95831b188c3192a2cf0127fb7b5136deb762a6b0b7bbdfb0dc6a40caf48251a62b164fffb81dd562bdd15ec3c8

practicalswift · 2020-09-08T04:57:15Z

A post-merge success story: only 21 days after merge -Wlogical-op caught the first bug in the form of #19912 :)

wallet/scriptpubkeyman.cpp:455:55: warning: logical ‘and’ applied to non-boolean constant [-Wlogical-op]
  455 |     if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT) && CHDChain::VERSION_HD_CHAIN_SPLIT) {
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Thanks a lot for reporting @kristapsk! ❤️

Perhaps we should do -Werror=logical-op.

Summary: > as discussed in [[ bitcoin/bitcoin#17344 | #17344]] > > 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). This is a backport of [[bitcoin/bitcoin#19015 | core#19015]] Disable `-Wduplicated-branches` for secp256k1, because of some exotic intentional use of ternary operator duplicated branches in test_inverse_scalar. Test Plan: ``` cmake .. -GNinja -DCMAKE_CXX_FLAGS=-Werror -DCMAKE_C_FLAGS=-Werror ninja && ninja check && ninja check-secp256k1 rm -Rf * cmake .. -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_CXX_FLAGS=-Werror -DCMAKE_C_FLAGS=-Werror ninja && ninja check && ninja check-secp256k1 ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10113

fanquake added the Build system label May 19, 2020

fanquake mentioned this pull request May 19, 2020

Fix -Wunreachable-code-loop-increment warning #19017

Closed

maflcko added the DrahtBot Guix build requested label May 21, 2020

DrahtBot removed the DrahtBot Guix build requested label May 23, 2020

maflcko added the DrahtBot Guix build requested label May 23, 2020

DrahtBot mentioned this pull request May 25, 2020

build: avoid repetitions when enabling warnings in configure.ac #18857

Closed

DrahtBot removed the DrahtBot Guix build requested label May 25, 2020

maflcko added the Needs gitian build label May 26, 2020

bitcoin deleted a comment from DrahtBot May 26, 2020

DrahtBot removed the Needs gitian build label May 28, 2020

bitcoin deleted a comment from DrahtBot May 28, 2020

maflcko added the Needs gitian build label May 28, 2020

DrahtBot removed the Needs gitian build label May 29, 2020

jonathanschoeller mentioned this pull request Jun 1, 2020

refactor: Fix unreachable code in init arg checks #19131

Merged

jonathanschoeller pushed a commit to jonathanschoeller/bitcoin that referenced this pull request Jun 1, 2020

build: Enable unreachable-code-loop-increment

eea8114

Enable unreachable-code-loop-increment and treat as error. refs: bitcoin#19015

bitcoin deleted a comment from DrahtBot Jun 5, 2020

maflcko added the Waiting for author label Jun 5, 2020

practicalswift force-pushed the compiler-diagnostics branch from 9d54881 to 7547a27 Compare June 7, 2020 19:40

maflcko removed the Waiting for author label Jun 7, 2020

practicalswift force-pushed the compiler-diagnostics branch from 7547a27 to f573cc2 Compare June 8, 2020 15:04

stackman27 pushed a commit to stackman27/bitcoin that referenced this pull request Jun 26, 2020

build: Enable unreachable-code-loop-increment

2d9b7d8

Enable unreachable-code-loop-increment and treat as error. refs: bitcoin#19015

maflcko added the Needs gitian build label Jul 2, 2020

DrahtBot removed the Needs gitian build label Jul 3, 2020

bitcoin deleted a comment from DrahtBot Aug 10, 2020

maflcko added the Waiting for author label Aug 10, 2020

build: Enable some commonly enabled compiler diagnostics

2f8a4c9

practicalswift force-pushed the compiler-diagnostics branch from f573cc2 to 2f8a4c9 Compare August 11, 2020 12:04

practicalswift mentioned this pull request Aug 12, 2020

Enable unused member function diagnostic by default #19702

Closed

fanquake removed the Waiting for author label Aug 14, 2020

hebasto approved these changes Aug 15, 2020

View reviewed changes

jonatack reviewed Aug 15, 2020

View reviewed changes

fanquake approved these changes Aug 18, 2020

View reviewed changes

fanquake merged commit 772cb03 into bitcoin:master Aug 18, 2020

kristapsk mentioned this pull request Sep 7, 2020

-Wlogical-op warning in wallet/scriptpubkeyman.cpp when building current master #19912

Closed

practicalswift deleted the compiler-diagnostics branch April 10, 2021 19:42

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Enable some commonly enabled compiler diagnostics #19015

build: Enable some commonly enabled compiler diagnostics #19015

practicalswift commented May 19, 2020 •

edited

maflcko commented May 19, 2020

hebasto commented May 19, 2020

DrahtBot commented May 22, 2020 •

edited

hebasto commented Jun 4, 2020

practicalswift commented Jun 5, 2020 •

edited

practicalswift commented Jun 7, 2020

maflcko commented Jun 7, 2020

practicalswift commented Jun 7, 2020

practicalswift commented Jun 8, 2020

maflcko commented Jun 8, 2020

fanquake commented Aug 10, 2020

practicalswift commented Aug 11, 2020 •

edited

fanquake commented Aug 11, 2020

practicalswift commented Aug 11, 2020

hebasto left a comment

jonatack left a comment •

edited

fanquake left a comment

practicalswift commented Sep 8, 2020

build: Enable some commonly enabled compiler diagnostics #19015

build: Enable some commonly enabled compiler diagnostics #19015

Conversation

practicalswift commented May 19, 2020 • edited

maflcko commented May 19, 2020

hebasto commented May 19, 2020

DrahtBot commented May 22, 2020 • edited

Conflicts

hebasto commented Jun 4, 2020

practicalswift commented Jun 5, 2020 • edited

practicalswift commented Jun 7, 2020

maflcko commented Jun 7, 2020

practicalswift commented Jun 7, 2020

practicalswift commented Jun 8, 2020

maflcko commented Jun 8, 2020

fanquake commented Aug 10, 2020

practicalswift commented Aug 11, 2020 • edited

fanquake commented Aug 11, 2020

practicalswift commented Aug 11, 2020

hebasto left a comment

Choose a reason for hiding this comment

jonatack left a comment • edited

Choose a reason for hiding this comment

fanquake left a comment

Choose a reason for hiding this comment

practicalswift commented Sep 8, 2020

practicalswift commented May 19, 2020 •

edited

DrahtBot commented May 22, 2020 •

edited

practicalswift commented Jun 5, 2020 •

edited

practicalswift commented Aug 11, 2020 •

edited

jonatack left a comment •

edited