Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 30, 2020

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.

@practicalswift
Copy link
Contributor

Concept ACK: Prefer class enums over “plain” enums (C++ Core Guidelines)

These are the remaining non-class enums that might be worth investigating too:

$ git grep '^enum [^c][^l][^(]*$' -- "*.h" "*.cpp" ":(exclude)src/univalue/" 
      ":(exclude)src/leveldb/" | cut -f2 -d' ' | cut -f1 -d: | sort -u | tr "\n" " "
BindFlags ChangeType DeploymentPos DisconnectResult GetDataMsg HTTPStatusCode
isminetype NetPermissionFlags Network NumConnections opcodetype RPCErrorCode SafeChars
ServiceFlags SOCKS5Atyp SOCKS5Command SOCKS5Method SOCKS5Reply SOCKSVersion txnouttype
WalletFeature WalletFlags

@hebasto
Copy link
Member

hebasto commented May 30, 2020

Concept ACK.
Also: #17877

@maflcko
Copy link
Member Author

maflcko commented May 30, 2020

@practicalswift It might be best to open a new brainstorming issue for discussion about those. I think enum flags can not (or should not?) be switched to enum class.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@maflcko maflcko force-pushed the 2005-enumClassTxoutType branch 2 times, most recently from fa69f11 to fa5997b Compare May 30, 2020 16:09
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa5997bd6fc82e16b597ea96e3c5c665f1f174ab, since fa38a7472ab475806cf50f302db9c69dfb8ac0f5 suggested changes applied.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 30, 2020

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

Conflicts

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 added 4 commits June 21, 2020 06:40
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-
@maflcko maflcko force-pushed the 2005-enumClassTxoutType branch from fa5997b to fa32adf Compare June 21, 2020 10:42
@maflcko
Copy link
Member Author

maflcko commented Jun 21, 2020

Rebased due to merge conflict in adjacent line. Can be trivially re-ACKed with git range-diff

@practicalswift
Copy link
Contributor

ACK fa32adf -- patch looks correct

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK fa32adf, since fa5997bd6fc82e16b597ea96e3c5c665f1f174ab (#19114 (review)) rebased only (verified with git range-diff).

@hebasto
Copy link
Member

hebasto commented Jun 22, 2020

@MarcoFalke from which key-server I could receive your non-expired key to verify commit signatures?

@maflcko
Copy link
Member Author

maflcko commented Jun 28, 2020

@hebasto See #18385

@hebasto
Copy link
Member

hebasto commented Jun 28, 2020

@MarcoFalke

@hebasto See #18385

Thanks. It was helpful!

@maflcko maflcko merged commit d3a5dbf into bitcoin:master Jun 28, 2020
@maflcko maflcko deleted the 2005-enumClassTxoutType branch June 28, 2020 18:22
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 2, 2021
Summary:
```
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.
```

Backport of core [[bitcoin/bitcoin#19114 | PR19114]].

Depends on D9124.

Test Plan:
  ninja all check-all
  ninja bitcoin-fuzzers

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9126
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants