From de28a0e10c73c0239fa5a623634c3a3acee02c57 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 2 Dec 2020 20:22:42 +0800 Subject: [PATCH 1/4] Merge #20530: lint, refactor: Update cppcheck linter to c++17 and improve explicit usage 1e62350ca20898189904a88dfef9ea11ddcd8626 refactor: Improve use of explicit keyword (Fabian Jahr) c502a6dbfb854ca827a5a3925394f9e09d29b898 lint: Use c++17 std in cppcheck linter (Fabian Jahr) Pull request description: I found the `extended-lint-cppcheck` linter still uses `std=c++11` when reviewing #20471. The only difference in the output after this change is one line is missing: ``` src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor] ``` After some digging, I am still not sure why this one is ignored with c++17 when 40 other`noExplicitConstructor` warnings were still appearing. In the second commit, I fix these warnings, adding `explicit` where appropriate and adding fixes to ignore otherwise. ACKs for top commit: practicalswift: cr ACK 1e62350ca20898189904a88dfef9ea11ddcd8626: patch looks correct! MarcoFalke: review ACK 1e62350ca20898189904a88dfef9ea11ddcd8626 Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12 --- src/node/interfaces.cpp | 2 +- src/qt/test/addressbooktests.h | 2 +- src/qt/test/rpcnestedtests.h | 2 +- src/qt/test/wallettests.h | 2 +- src/rpc/request.h | 2 +- src/script/descriptor.cpp | 2 +- src/test/fuzz/signature_checker.cpp | 2 +- src/test/util/logging.h | 2 +- src/test/util_tests.cpp | 2 +- src/tinyformat.h | 4 ++-- src/txmempool.h | 2 +- src/util/hash_type.h | 2 +- src/validation.cpp | 2 +- src/wallet/bdb.h | 2 +- src/wallet/scriptpubkeyman.h | 4 ++-- test/lint/extended-lint-cppcheck.sh | 11 ++++++++++- 16 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f7e2056895..6a3b648e8a 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -194,7 +194,7 @@ public: MasternodeSyncImpl m_masternodeSync; CoinJoinOptionsImpl m_coinjoin; - NodeImpl(NodeContext* context) { setContext(context); } + explicit NodeImpl(NodeContext* context) { setContext(context); } void initError(const bilingual_str& message) override { InitError(message); } bool parseParameters(int argc, const char* const argv[], std::string& error) override { diff --git a/src/qt/test/addressbooktests.h b/src/qt/test/addressbooktests.h index 5de89c7592..e6d24f202f 100644 --- a/src/qt/test/addressbooktests.h +++ b/src/qt/test/addressbooktests.h @@ -15,7 +15,7 @@ class Node; class AddressBookTests : public QObject { public: - AddressBookTests(interfaces::Node& node) : m_node(node) {} + explicit AddressBookTests(interfaces::Node& node) : m_node(node) {} interfaces::Node& m_node; Q_OBJECT diff --git a/src/qt/test/rpcnestedtests.h b/src/qt/test/rpcnestedtests.h index b15c62d25b..2700a48c98 100644 --- a/src/qt/test/rpcnestedtests.h +++ b/src/qt/test/rpcnestedtests.h @@ -15,7 +15,7 @@ class Node; class RPCNestedTests : public QObject { public: - RPCNestedTests(interfaces::Node& node) : m_node(node) {} + explicit RPCNestedTests(interfaces::Node& node) : m_node(node) {} interfaces::Node& m_node; Q_OBJECT diff --git a/src/qt/test/wallettests.h b/src/qt/test/wallettests.h index 8ee40bf07f..dfca3370f7 100644 --- a/src/qt/test/wallettests.h +++ b/src/qt/test/wallettests.h @@ -15,7 +15,7 @@ class Node; class WalletTests : public QObject { public: - WalletTests(interfaces::Node& node) : m_node(node) {} + explicit WalletTests(interfaces::Node& node) : m_node(node) {} interfaces::Node& m_node; Q_OBJECT diff --git a/src/rpc/request.h b/src/rpc/request.h index b076403936..71fe43a047 100644 --- a/src/rpc/request.h +++ b/src/rpc/request.h @@ -38,7 +38,7 @@ public: std::string peerAddr; const CoreContext& context; - JSONRPCRequest(const CoreContext& context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {} + explicit JSONRPCRequest(const CoreContext& context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {} //! Initializes request information from another request object and the //! given context. The implementation should be updated if any members are diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index ef1a7c5b74..ae540d66b5 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -157,7 +157,7 @@ protected: uint32_t m_expr_index; public: - PubkeyProvider(uint32_t exp_index) : m_expr_index(exp_index) {} + explicit PubkeyProvider(uint32_t exp_index) : m_expr_index(exp_index) {} virtual ~PubkeyProvider() = default; diff --git a/src/test/fuzz/signature_checker.cpp b/src/test/fuzz/signature_checker.cpp index 6c05c7482e..c0cecf53f1 100644 --- a/src/test/fuzz/signature_checker.cpp +++ b/src/test/fuzz/signature_checker.cpp @@ -23,7 +23,7 @@ class FuzzedSignatureChecker : public BaseSignatureChecker FuzzedDataProvider& m_fuzzed_data_provider; public: - FuzzedSignatureChecker(FuzzedDataProvider& fuzzed_data_provider) : m_fuzzed_data_provider(fuzzed_data_provider) + explicit FuzzedSignatureChecker(FuzzedDataProvider& fuzzed_data_provider) : m_fuzzed_data_provider(fuzzed_data_provider) { } diff --git a/src/test/util/logging.h b/src/test/util/logging.h index 1fcf7ca305..a49f9a7292 100644 --- a/src/test/util/logging.h +++ b/src/test/util/logging.h @@ -32,7 +32,7 @@ class DebugLogHelper void check_found(); public: - DebugLogHelper(std::string message, MatchFn match = [](const std::string*){ return true; }); + explicit DebugLogHelper(std::string message, MatchFn match = [](const std::string*){ return true; }); ~DebugLogHelper() { check_found(); } }; diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 2b9cb2f976..38b8c5acf2 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -239,7 +239,7 @@ public: std::optional> list_value; const char* error = nullptr; - Expect(util::SettingsValue s) : setting(std::move(s)) {} + explicit Expect(util::SettingsValue s) : setting(std::move(s)) {} Expect& DefaultString() { default_string = true; return *this; } Expect& DefaultInt() { default_int = true; return *this; } Expect& DefaultBool() { default_bool = true; return *this; } diff --git a/src/tinyformat.h b/src/tinyformat.h index f75ec7cfca..0211b52600 100644 --- a/src/tinyformat.h +++ b/src/tinyformat.h @@ -514,7 +514,7 @@ class FormatArg { } template - FormatArg(const T& value) + explicit FormatArg(const T& value) : m_value(static_cast(&value)), m_formatImpl(&formatImpl), m_toIntImpl(&toIntImpl) @@ -970,7 +970,7 @@ class FormatListN : public FormatList public: #ifdef TINYFORMAT_USE_VARIADIC_TEMPLATES template - FormatListN(const Args&... args) + explicit FormatListN(const Args&... args) : FormatList(&m_formatterStore[0], N), m_formatterStore { FormatArg(args)... } { static_assert(sizeof...(args) == N, "Number of args must be N"); } diff --git a/src/txmempool.h b/src/txmempool.h index cde068bdcf..8278ab0f4e 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -840,7 +840,7 @@ public: class EpochGuard { const CTxMemPool& pool; public: - EpochGuard(const CTxMemPool& in); + explicit EpochGuard(const CTxMemPool& in); ~EpochGuard(); }; // N.B. GetFreshEpoch modifies mutable state via the EpochGuard construction diff --git a/src/util/hash_type.h b/src/util/hash_type.h index a3afcd3186..96771546dc 100644 --- a/src/util/hash_type.h +++ b/src/util/hash_type.h @@ -13,7 +13,7 @@ protected: public: BaseHash() : m_hash() {} - BaseHash(const HashType& in) : m_hash(in) {} + explicit BaseHash(const HashType& in) : m_hash(in) {} unsigned char* begin() { diff --git a/src/validation.cpp b/src/validation.cpp index add1602cc5..77687552b2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -566,7 +566,7 @@ private: // All the intermediate state that gets passed between the various levels // of checking a given transaction. struct Workspace { - Workspace(const CTransactionRef& ptx) : m_ptx(ptx), m_hash(ptx->GetHash()) {} + explicit Workspace(const CTransactionRef& ptx) : m_ptx(ptx), m_hash(ptx->GetHash()) {} CTxMemPool::setEntries m_ancestors; std::unique_ptr m_entry; diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index f263d40f66..acfaf6686f 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -56,7 +56,7 @@ public: std::unordered_map m_fileids; std::condition_variable_any m_db_in_use; - BerkeleyEnvironment(const fs::path& env_directory); + explicit BerkeleyEnvironment(const fs::path& env_directory); BerkeleyEnvironment(); ~BerkeleyEnvironment(); void Reset(); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 79cdb1729a..b34689a6b3 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -148,7 +148,7 @@ protected: WalletStorage& m_storage; public: - ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {} + explicit ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {} virtual ~ScriptPubKeyMan() {}; virtual bool GetNewDestination(CTxDestination& dest, std::string& error) { return false; } @@ -486,7 +486,7 @@ class LegacySigningProvider : public SigningProvider private: const LegacyScriptPubKeyMan& m_spk_man; public: - LegacySigningProvider(const LegacyScriptPubKeyMan& spk_man) : m_spk_man(spk_man) {} + explicit LegacySigningProvider(const LegacyScriptPubKeyMan& spk_man) : m_spk_man(spk_man) {} bool GetCScript(const CScriptID &scriptid, CScript& script) const override { return m_spk_man.GetCScript(scriptid, script); } bool HaveCScript(const CScriptID &scriptid) const override { return m_spk_man.HaveCScript(scriptid); } diff --git a/test/lint/extended-lint-cppcheck.sh b/test/lint/extended-lint-cppcheck.sh index 8c001754e1..67cb45b8fe 100755 --- a/test/lint/extended-lint-cppcheck.sh +++ b/test/lint/extended-lint-cppcheck.sh @@ -30,6 +30,7 @@ IGNORED_WARNINGS=( "src/protocol.h:.* Class 'CMessageHeader' has a constructor with 1 argument that is not explicit." "src/qt/guiutil.h:.* Class 'ItemDelegate' has a constructor with 1 argument that is not explicit." "src/rpc/util.h:.* Struct 'RPCResults' has a constructor with 1 argument that is not explicit." + "src/rpc/util.h:.* Struct 'UniValueType' has a constructor with 1 argument that is not explicit." "src/rpc/util.h:.* style: Struct 'UniValueType' has a constructor with 1 argument that is not explicit." "src/script/descriptor.cpp:.* Class 'AddressDescriptor' has a constructor with 1 argument that is not explicit." "src/script/descriptor.cpp:.* Class 'ComboDescriptor' has a constructor with 1 argument that is not explicit." @@ -42,6 +43,11 @@ IGNORED_WARNINGS=( "src/script/descriptor.cpp:.* Class 'WSHDescriptor' has a constructor with 1 argument that is not explicit." "src/script/script.h:.* Class 'CScript' has a constructor with 1 argument that is not explicit." "src/script/standard.h:.* Class 'CScriptID' has a constructor with 1 argument that is not explicit." + "src/span.h:.* Class 'Span < const CRPCCommand >' has a constructor with 1 argument that is not explicit." + "src/span.h:.* Class 'Span < const char >' has a constructor with 1 argument that is not explicit." + "src/span.h:.* Class 'Span < const std :: vector >' has a constructor with 1 argument that is not explicit." + "src/span.h:.* Class 'Span < const uint8_t >' has a constructor with 1 argument that is not explicit." + "src/span.h:.* Class 'Span' has a constructor with 1 argument that is not explicit." "src/support/allocators/secure.h:.* Struct 'secure_allocator < char >' has a constructor with 1 argument that is not explicit." "src/support/allocators/secure.h:.* Struct 'secure_allocator < RNGState >' has a constructor with 1 argument that is not explicit." "src/support/allocators/secure.h:.* Struct 'secure_allocator < unsigned char >' has a constructor with 1 argument that is not explicit." @@ -49,6 +55,9 @@ IGNORED_WARNINGS=( "src/test/checkqueue_tests.cpp:.* Struct 'FailingCheck' has a constructor with 1 argument that is not explicit." "src/test/checkqueue_tests.cpp:.* Struct 'MemoryCheck' has a constructor with 1 argument that is not explicit." "src/test/checkqueue_tests.cpp:.* Struct 'UniqueCheck' has a constructor with 1 argument that is not explicit." + "src/test/fuzz/util.h:.* Class 'FuzzedFileProvider' has a constructor with 1 argument that is not explicit." + "src/test/fuzz/util.h:.* Class 'FuzzedAutoFileProvider' has a constructor with 1 argument that is not explicit." + "src/util/ref.h:.* Class 'Ref' has a constructor with 1 argument that is not explicit." "src/wallet/db.h:.* Class 'BerkeleyEnvironment' has a constructor with 1 argument that is not explicit." ) @@ -66,7 +75,7 @@ function join_array { ENABLED_CHECKS_REGEXP=$(join_array "|" "${ENABLED_CHECKS[@]}") IGNORED_WARNINGS_REGEXP=$(join_array "|" "${IGNORED_WARNINGS[@]}") WARNINGS=$(git ls-files -- "*.cpp" "*.h" ":(exclude)src/dashbls/" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/" | \ - xargs cppcheck --enable=all -j "$(getconf _NPROCESSORS_ONLN)" --language=c++ --std=c++11 --template=gcc -D__cplusplus -DCLIENT_VERSION_BUILD -DCLIENT_VERSION_IS_RELEASE -DCLIENT_VERSION_MAJOR -DCLIENT_VERSION_MINOR -DCOPYRIGHT_YEAR -DDEBUG -I src/ -q 2>&1 | sort -u | \ + xargs cppcheck --enable=all -j "$(getconf _NPROCESSORS_ONLN)" --language=c++ --std=c++17 --template=gcc -D__cplusplus -DCLIENT_VERSION_BUILD -DCLIENT_VERSION_IS_RELEASE -DCLIENT_VERSION_MAJOR -DCLIENT_VERSION_MINOR -DCOPYRIGHT_YEAR -DDEBUG -I src/ -q 2>&1 | sort -u | \ grep -E "${ENABLED_CHECKS_REGEXP}" | \ grep -vE "${IGNORED_WARNINGS_REGEXP}") if [[ ${WARNINGS} != "" ]]; then From 84b8d62d345b81bebfb52a6f2c1026f12abfe2f9 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 3 Dec 2020 14:52:23 +0100 Subject: [PATCH 2/4] Merge #20221: net: compat.h related cleanup cadb77a6ab8a3e6f56062cfaec4dd8168c71b39d net: Add compat.h header for htonl function (Hennadii Stepanov) f796f0057bc7dad8e7065831b07f432fc0fb9f08 net: Drop unneeded headers when compat.h included (Hennadii Stepanov) 467c34644861a5267601255650e27c7aadab31dc net: Drop unneeded Windows headers in compat.h (Hennadii Stepanov) Pull request description: It is the `compat.h` header's job to provide platform-agnostic interfaces for internet operations. No need in `#include ` scattered around. ACKs for top commit: practicalswift: re-ACK cadb77a6ab8a3e6f56062cfaec4dd8168c71b39d: patch looks even better laanwj: Code review ACK cadb77a6ab8a3e6f56062cfaec4dd8168c71b39d Tree-SHA512: 625ff90b2806310ab856a6ca1ddb6d9a85aa70f342b323e8525a711dd12219a1ecec8373ec1dca5a0653ffb11f9b421753887b25615d991ba3132c1cca6a3c6e --- src/compat.h | 6 +----- src/httpserver.cpp | 7 ------- src/net.h | 5 ----- src/torcontrol.cpp | 1 + 4 files changed, 2 insertions(+), 17 deletions(-) diff --git a/src/compat.h b/src/compat.h index f790b867eb..cf074ca1d0 100644 --- a/src/compat.h +++ b/src/compat.h @@ -21,11 +21,7 @@ #undef FD_SETSIZE // prevent redefinition compiler warning #endif #define FD_SETSIZE 1024 // max number of fds in fd_set - -#include // Must be included before mswsock.h and windows.h - -#include -#include +#include #include #include #else diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 28e1289ce6..1d4af8d3f0 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -29,13 +29,6 @@ #include -#ifdef EVENT__HAVE_NETINET_IN_H -#include -#ifdef _XOPEN_SOURCE_EXTENDED -#include -#endif -#endif - #include #include diff --git a/src/net.h b/src/net.h index 359c00b795..aee79057a6 100644 --- a/src/net.h +++ b/src/net.h @@ -40,11 +40,6 @@ #include #include -#ifndef WIN32 -#include -#endif - - #ifndef WIN32 #define USE_WAKEUP_PIPE #endif diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index 351d124704..c60506fb5e 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include #include From dbf605e1f92f683b69193336299111cba1fecce6 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 4 Dec 2020 15:18:44 +0100 Subject: [PATCH 3/4] (Partial) Merge #20566: refactor: Use C++17 std::array where possible fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2 refactor: Use C++17 std::array where possible (MarcoFalke) Pull request description: Using the C++11 std::array with explicit template parameters is problematic because overshooting the size will fill the memory with default constructed types. For example, ```cpp #include #include int main() { std::array a{1, 2}; for (const auto& i : a) { std::cout << i << std::endl; // prints "1 2 0" } } ``` ACKs for top commit: jonasschnelli: Code Review ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2 practicalswift: cr ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2 vasild: ACK fac7ab1d promag: Code review ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2. Tree-SHA512: ef7e872340226e0d6160e6fd66c6ca78b2ef9c245fa0ab27fe4777aac9fba8d5aaa154da3d27b65dec39a6a63d07f1063c3a8ffb667a98ab137756a1a0af2656 --- src/qt/sendcoinsdialog.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 87dfd29be3..410e537280 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -35,7 +35,7 @@ #define SEND_CONFIRM_DELAY 3 -static const std::array confTargets = { {2, 4, 6, 12, 24, 48, 144, 504, 1008} }; +static constexpr std::array confTargets{2, 4, 6, 12, 24, 48, 144, 504, 1008}; int getConfTargetForIndex(int index) { if (index+1 > static_cast(confTargets.size())) { return confTargets.back(); From 5b0e3b9f30be22bab877ef23a6860fc284388f1c Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 7 Dec 2020 09:07:53 +0100 Subject: [PATCH 4/4] Merge #19832: p2p: Put disconnecting logs into BCLog::NET category 1816327e533d359c237c53eb6440b2f3a7cbf4fa p2p: Put disconnecting logs into BCLog::NET category (Hennadii Stepanov) Pull request description: It's too noisy: ``` $ cat debug.log | wc -l 28529 $ cat debug.log | grep "Disconnecting and discouraging peer" | wc -l 10177 ``` ACKs for top commit: MarcoFalke: noban, addnode and local peers are still unconditionally logged (as they should), but this one can go into a category, so cr-ACK 1816327e533d359c237c53eb6440b2f3a7cbf4fa practicalswift: ACK 1816327e533d359c237c53eb6440b2f3a7cbf4fa for the reasons MarcoFalke gave above. ajtowns: ACK 1816327e533d359c237c53eb6440b2f3a7cbf4fa Tree-SHA512: c312c1009090840659b2cb1364d8ad9b6ab8e742fc462aef169996d93c76c248507639a00257ed9d73a6916c01176b1793491b2305e92fdded5f9de0935b6ba6 --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 78344eaeec..0effb61250 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4387,7 +4387,7 @@ bool PeerManagerImpl::MaybeDiscourageAndDisconnect(CNode& pnode) } // Disconnect and discourage all nodes sharing the address - LogPrintf("Disconnecting and discouraging peer %d!\n", peer_id); + LogPrint(BCLog::NET, "Disconnecting and discouraging peer %d!\n", peer_id); if (m_banman) { m_banman->Discourage(pnode.addr); }