Non-scoped enums can accidentally and silently decay into an integral type. Also, the symbol names of the keys are exported to the surrounding (usually global) namespace.
Fix both issues by switching to an enum class TxoutType in a (mostly) scripted-diff.
Concept ACK: Prefer class enums over “plain” enums (C++ Core Guidelines)
These are the remaining non-class enums that might be worth investigating too:
0$ git grep '^enum [^c][^l][^(]*$' -- "*.h" "*.cpp" ":(exclude)src/univalue/"
1 ":(exclude)src/leveldb/" | cut -f2 -d' ' | cut -f1 -d: | sort -u | tr "\n" " "
2BindFlags ChangeType DeploymentPos DisconnectResult GetDataMsg HTTPStatusCode
3isminetype NetPermissionFlags Network NumConnections opcodetype RPCErrorCode SafeChars
4ServiceFlags SOCKS5Atyp SOCKS5Command SOCKS5Method SOCKS5Reply SOCKSVersion txnouttype
5WalletFeature WalletFlags
35@@ -36,11 +36,11 @@ static const unsigned int MAX_OP_RETURN_RELAY = 83;
36
37 /**
38 * A data carrying output is an unspendable output containing data. The script
39- * type is designated as TX_NULL_DATA.
40+ * type is designated as NULL_DATA.
nit:
0 * type is designated as TxoutType::NULL_DATA.
40+ * type is designated as NULL_DATA.
41 */
42 extern bool fAcceptDatacarrier;
43
44-/** Maximum size of TX_NULL_DATA scripts that this node considers standard. */
45+/** Maximum size of NULL_DATA scripts that this node considers standard. */
nit:
0/** Maximum size of TxoutType::NULL_DATA scripts that this node considers standard. */
155@@ -157,9 +156,9 @@ std::string GetTxnOutputType(txnouttype t);
156 *
157 * @param[in] scriptPubKey Script to parse
158 * @param[out] vSolutionsRet Vector of parsed pubkeys and hashes
159- * @return The script type. TX_NONSTANDARD represents a failed solve.
160+ * @return The script type. NONSTANDARD represents a failed solve.
nit:
0 * [@return](/bitcoin-bitcoin/contributor/return/) The script type. TxoutType::NONSTANDARD represents a failed solve.
enum class.
49+ case TxoutType::NULL_DATA: return "nulldata";
50+ case TxoutType::WITNESS_V0_KEYHASH: return "witness_v0_keyhash";
51+ case TxoutType::WITNESS_V0_SCRIPTHASH: return "witness_v0_scripthash";
52+ case TxoutType::WITNESS_UNKNOWN: return "witness_unknown";
53+ // no default case, so the compiler can warn about missing cases
54 }
nit:
0 } // no default case, so the compiler can warn about missing cases
as described in Developer Notes
ACK fa38a7472ab475806cf50f302db9c69dfb8ac0f5, tested on Linux Mint 19.3 (x86_64).
fa868d3a8de56582538a3e4ef8de6082f2b8c747: maybe make a separate utility function to derive an underlying type of an enumerator?
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.
This commit does not change behavior
Don't blindly assume it is int.
In practice this is usually `unsigned` or `int`, so this commit should
not change behavior.
Also, remove scope of txnouttype in fuzz tests temporarily. The next
commit will add scopes to all txnouttype.
-BEGIN VERIFY SCRIPT-
# General rename helper: $1 -> $2
rename_global() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1"); }
# Helper to rename TxoutType $1
rename_value() {
sed -i "s/ TX_$1,/ $1,/g" src/script/standard.h; # First strip the prefix in the definition (header)
rename_global TX_$1 "TxoutType::$1"; # Then replace globally
}
# Change the type globally to bring it in line with the style-guide
# (clsses are UpperCamelCase)
rename_global 'enum txnouttype' 'enum class TxoutType'
rename_global 'txnouttype' 'TxoutType'
# Now rename each enum value
rename_value 'NONSTANDARD'
rename_value 'PUBKEY'
rename_value 'PUBKEYHASH'
rename_value 'SCRIPTHASH'
rename_value 'MULTISIG'
rename_value 'NULL_DATA'
rename_value 'WITNESS_V0_KEYHASH'
rename_value 'WITNESS_V0_SCRIPTHASH'
rename_value 'WITNESS_UNKNOWN'
-END VERIFY SCRIPT-Rebased due to merge conflict in adjacent line. Can be trivially re-ACKed with git range-diff
git range-diff).