Make single parameter constructors explicit (C++11).
Rationale from the developer notes:
- By default, declare single-argument constructors
explicit.
- Rationale: This is a precaution to avoid unintended conversions that might arise when single-argument constructors are used as implicit conversion functions.
@promag Perhaps surprisingly clang and GCC lack such a warning AFAIK, but the Intel C++ Compiler has it.
Also most C++ linters support flagging single parameter constructors not declared explicit.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
explicit. Please re-review :-)
@promag Would you mind reviewing the updated version which contains the automatic Travis checking for explicit?
If merged we’ll never have to think about explicit ever again :-)
16+ "src/prevector.h:.* Class 'const_reverse_iterator' has a constructor with 1 argument that is not explicit."
17+ "src/prevector.h:.* Class 'iterator' has a constructor with 1 argument that is not explicit."
18+ "src/prevector.h:.* Class 'reverse_iterator' has a constructor with 1 argument that is not explicit."
19+ "src/primitives/transaction.h:.* Class 'CTransaction' has a constructor with 1 argument that is not explicit."
20+ "src/protocol.h:.* Class 'CMessageHeader' has a constructor with 1 argument that is not explicit."
21+ "src/rpc/server.h:.* Struct 'UniValueType' has a constructor with 1 argument that is not explicit."
I excluded the constructors that appeared to be intentionally non-explicit or where switching to explicit seemed non-trivial.
The remaining cases are:
0CTransaction(CMutableTransaction &&tx)
1CTransaction(const CMutableTransaction &tx)
2UniValueType(UniValue::VType _type)
3arith_uint256(const base_uint<256>& b)
4arith_uint256(uint64_t b)
5base_uint(uint64_t b)
6const_iterator(const T* ptr_)
7const_iterator(iterator x)
8const_reverse_iterator(const T* ptr_)
9const_reverse_iterator(reverse_iterator x)
10iterator(T* ptr_)
11reverse_iterator(T* ptr_)
12secure_allocator(const secure_allocator<U>& a)
13zero_after_free_allocator(const zero_after_free_allocator<U>& a)
And also two cases that appear to be incorrectly flagged as single-parameter:
0CMessageHeader(const MessageStartChars& pchMessageStartIn, const char* pszCommand, unsigned int nMessageSizeIn)
1CNetMessage(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn)
Should any of these be be turned explicit as part of this PR? :-)
36+}
37+
38+ENABLED_CHECKS_REGEXP=$(join_array "|" "${ENABLED_CHECKS[@]}")
39+IGNORED_WARNINGS_REGEXP=$(join_array "|" "${IGNORED_WARNINGS[@]}")
40+WARNINGS=$(git ls-files -- "*.cpp" "*.h" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/" | \
41+ xargs cppcheck --enable=all -j "$(nproc)" --language=c++ --std=c++11 --template=gcc -D__cplusplus -DCLIENT_VERSION_BUILD -DCLIENT_VERSION_IS_RELEASE -DCLIENT_VERSION_MAJOR -DCLIENT_VERSION_MINOR -DCLIENT_VERSION_REVISION -DCOPYRIGHT_YEAR -DDEBUG -DHAVE_WORKING_BOOST_SLEEP_FOR -I src/ -q 2>&1 | \
cppcheck in this PR :-)
@luke-jr In this specific case the alternative and the current situation is that a human reviewer with probability ~100 % will point out that the PR author forgot to add the explicit specifier. The PR author will then add it with probability ~100 % before PR merge since explicit is the reasonable post-C++11 default which we explicitly recommend in the developer notes:
By default, declare single-argument constructors explicit.
Rationale: This is a precaution to avoid unintended conversions that might arise when single-argument constructors are used as implicit conversion functions.
This is one of the most common review comments in my experience. IMO it is better to have it automatically and mechanically fixed by the PR author before human review takes place to minimise the time spent by human reviewers on dull “mechanical review work”.
Rationale directly from the Bible’s Old Testament^W^W^WC++ Core Guidelines with editors Bjarne Stroustrup and Herb Sutter:
C.46: By default, declare single-argument constructors explicit … Reason: To avoid unintended conversions. … Enforcement: (Simple) Single-argument constructors should be declared explicit. Good single argument non-explicit constructors are rare in most code based. Warn for all that are not on a “positive list”.
:-)
61+ install:
62+ - set -o errexit; source .travis/extended_lint_04_install.sh
63+ before_script:
64+ - set -o errexit; source .travis/lint_05_before_script.sh
65+ script:
66+ - set -o errexit; source .travis/extended_lint_06_script.sh
55+ - stage: extended-lint
56+ name: 'extended lint [runtime >= 60 seconds]'
57+ env:
58+ cache: false
59+ language: python
60+ python: '3.6'
lint?
0@@ -0,0 +1,12 @@
1+#!/usr/bin/env bash
2+#
3+# Copyright (c) 2018 The Bitcoin Core developers
4+# Distributed under the MIT software license, see the accompanying
5+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
6+
7+export LC_ALL=C
8+
9+CPPCHECK_VERSION=1.85
0@@ -0,0 +1,12 @@
1+#!/usr/bin/env bash
2+#
3+# Copyright (c) 2018 The Bitcoin Core developers
2019 here, and in all the other new files.
0@@ -0,0 +1,71 @@
1+#!/usr/bin/env bash
57+}
58+
59+ENABLED_CHECKS_REGEXP=$(join_array "|" "${ENABLED_CHECKS[@]}")
60+IGNORED_WARNINGS_REGEXP=$(join_array "|" "${IGNORED_WARNINGS[@]}")
61+WARNINGS=$(git ls-files -- "*.cpp" "*.h" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/" | \
62+ xargs cppcheck --enable=all -j "$(nproc)" --language=c++ --std=c++11 --template=gcc -D__cplusplus -DCLIENT_VERSION_BUILD -DCLIENT_VERSION_IS_RELEASE -DCLIENT_VERSION_MAJOR -DCLIENT_VERSION_MINOR -DCLIENT_VERSION_REVISION -DCOPYRIGHT_YEAR -DDEBUG -DHAVE_WORKING_BOOST_SLEEP_FOR -I src/ -q 2>&1 | \
nproc isn’t available on macOS, although if this is script is just meant for Travis I guess it doesn’t matter too much.
sysctl -n hw.physicalcpu is a macOS nproc equivalent.
nproc is part of coreutils. I’ve now switched to getconf _NPROCESSORS_ONLN which is the most portable way to get the number of processors AFAIK.
24+ "src/prevector.h:.* Class 'const_iterator' has a constructor with 1 argument that is not explicit."
25+ "src/prevector.h:.* Class 'const_reverse_iterator' has a constructor with 1 argument that is not explicit."
26+ "src/prevector.h:.* Class 'iterator' has a constructor with 1 argument that is not explicit."
27+ "src/prevector.h:.* Class 'reverse_iterator' has a constructor with 1 argument that is not explicit."
28+ "src/primitives/block.h:.* Class 'CBlock' has a constructor with 1 argument that is not explicit."
29+ "src/primitives/transaction.h:.* Class 'CTransaction' has a constructor with 1 argument that is not explicit."
I’m afraid that suppressions is still needed in master:
0$ test/lint/extended-lint-cppcheck.sh
1src/primitives/transaction.h:302: style: Class 'CTransaction' has a constructor with 1 argument that is not explicit.
2
3Advice not applicable in this specific case? Add an exception by updating
4IGNORED_WARNINGS in test/lint/extended-lint-cppcheck.sh
5$ head -302 src/primitives/transaction.h | tail -1
6 CTransaction(CMutableTransaction &&tx);
@MarcoFalke Updated. Added suppression src/support/allocators/secure.h:.* style: Struct 'secure_allocator < RNGState >' has a constructor with 1 argument that is not explicit..
Please re-review :-) I think we’re ready for merge :-)
This is controversial in its current form. See #14505 (comment)
I guess I’d be ok with it if it would never fail (and only print the warnings), like the spell-checker.
53@@ -53,6 +54,19 @@ jobs:
54 script:
55 - set -o errexit; source .travis/lint_06_script.sh
56
57+ - stage: extended-lint
58+ name: 'extended lint [runtime >= 60 seconds]'
59+ env:
60+ cache: false
61+ language: python
62+ python: '3.4' # Oldest supported version according to doc/dependencies.md
0@@ -0,0 +1,26 @@
1+#!/usr/bin/env bash
2+#
3+# Copyright (c) 2019 The Bitcoin Core developers
4+# Distributed under the MIT software license, see the accompanying
5+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
6+#
7+# This script runs all contrib/devtools/lint-*.sh files, and fails if any exit
lint- -> extended-lint-
73+ echo "${WARNINGS}"
74+ echo
75+ echo "Advice not applicable in this specific case? Add an exception by updating"
76+ echo "IGNORED_WARNINGS in $0"
77+ # Uncomment to enforce the developer note policy "By default, declare single-argument constructors `explicit`"
78+ # exit 1
allow_failures travis flag?