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.
Concept ACK. @practicalswift is there a compiler flag to detect these?
@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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
@MarcoFalke Good point! I've now added a Travis check that will catch newly introduced single-argument constructors not declare 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 :-)
utACK c191011 - could squash the first five commits, and "all" is not strictly accurate given the linter exceptions.
@Empact Good points! Now addressed. Please re-review :-)
Rebased!
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."
Why are some of our own classes suppressed as warnings and others fixed up? Seems arbitrary to me.
I excluded the constructors that appeared to be intentionally non-explicit or where switching to explicit seemed non-trivial.
The remaining cases are:
CTransaction(CMutableTransaction &&tx)
CTransaction(const CMutableTransaction &tx)
UniValueType(UniValue::VType _type)
arith_uint256(const base_uint<256>& b)
arith_uint256(uint64_t b)
base_uint(uint64_t b)
const_iterator(const T* ptr_)
const_iterator(iterator x)
const_reverse_iterator(const T* ptr_)
const_reverse_iterator(reverse_iterator x)
iterator(T* ptr_)
reverse_iterator(T* ptr_)
secure_allocator(const secure_allocator<U>& a)
zero_after_free_allocator(const zero_after_free_allocator<U>& a)
And also two cases that appear to be incorrectly flagged as single-parameter:
CMessageHeader(const MessageStartChars& pchMessageStartIn, const char* pszCommand, unsigned int nMessageSizeIn)
CNetMessage(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn)
Should any of these be be turned explicit as part of this PR? :-)
Makes sense. Though, I'd prefer if this was a test only change (adding all the suppressions) and then go out and fixup violations in the code in another pull request(s), since both tasks are slightly different when it comes to review process.
Doesn't compile?
Rebased!
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 | \
Looks like installing and running this takes some time. Could add another more expensive lint check after the two stages as a third stage? (Feel free to ignore my suggestion)
@MarcoFalke Thanks for reviewing. Feedback addressed: 1.) this PR is now test only and 2.) added another more expensive lint check after the two stages as a third stage. Please re-review :-)
@promag I'm not sure the Intel C++ Compiler license would allow that. On the other hand the problem is already solved using cppcheck in this PR :-)
utACK f9d10fd2315dd73c8f1c8e6e18c041ebdffdd463
Am I the only one who finds it annoying when linters fail on non-bugs?
@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”.
:-)
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
Rebased! :-) @MarcoFalke @promag Would you mind re-reviewing?
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
nit: somewhat prefer the visual separation of whitespace
55 | + - stage: extended-lint 56 | + name: 'extended lint [runtime >= 60 seconds]' 57 | + env: 58 | + cache: false 59 | + language: python 60 | + python: '3.6'
Should this be 3.4, as with lint?
@Empact Thanks for reviewing. Feedback addressed. Please re-review :-)
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
I assume we can use Cppcheck 1.86? Might as well update to the newest version before merging.
0 | @@ -0,0 +1,12 @@ 1 | +#!/usr/bin/env bash 2 | +# 3 | +# Copyright (c) 2018 The Bitcoin Core developers
Can just use 2019 here, and in all the other new files.
0 | @@ -0,0 +1,71 @@ 1 | +#!/usr/bin/env bash
Missing copyright header.
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.
Good point: 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.
@fanquake Thanks for the review and feedback. Feedback addressed. Please re-review :-)
re-utACK 6907678
@MarcoFalke @promag Would you mind re-reviewing? :-)
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."
this is explicit in master?
I'm afraid that suppressions is still needed in master:
$ test/lint/extended-lint-cppcheck.sh
src/primitives/transaction.h:302: style: Class 'CTransaction' has a constructor with 1 argument that is not explicit.
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/extended-lint-cppcheck.sh
$ head -302 src/primitives/transaction.h | tail -1
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 :-)
Rebased and updated. @MarcoFalke @promag @Empact, would you mind reviewing? :-)
@MarcoFalke Can this one get a release milestone? :-)
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
Needs rebase (and update)
@MarcoFalke Updated. The updated version never fails: it only prints the warnings -- like the spell-checker. Looks good? :-)
utACK 2eb79c3601bfb4bff140693b3225aa4779d73390
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
nit: lint- -> extended-lint-
Fixed typo identified by @Empact and squashed. @Empact @MarcoFalke @promag Would you mind re-reviewing? :-)
ACK c4606b84329d760d7cee144bebe05807857edaae
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
It could be enabled, but guarded by a allow_failures travis flag?