scripted-diff: Use scoped enumerations (C++11, "enum class") #10742

practicalswift · 2017-07-04T15:48:26Z

Rationale (from Bjarne Stroustrup's "C++11 FAQ"):

The enum classes ("new enums", "strong enums") address three problems with traditional C++ enumerations:

conventional enums implicitly convert to int, causing errors when someone does not want an enumeration to act as an integer.

conventional enums export their enumerators to the surrounding scope, causing name clashes.

the underlying type of an enum cannot be specified, causing confusion, compatibility problems, and makes forward declaration impossible.

The new enums are "enum class" because they combine aspects of traditional enumerations (names values) with aspects of classes (scoped members and absence of conversions).

ArielLorenzo-Luaces · 2017-07-05T07:47:57Z

src/rpc/protocol.h

Using strongly typed enums causes RPCErrorCode to be static_casted too often. To gain strongly typed enums we loose readability.

Especially when considering that a more correct way to cast a strongly typed enum is static_cast<std::underlying_type<RPCErrorCode>::type>(RPCErrorCode::RPC_MISC_ERROR)
Which is even more unreadable.

Instead I suggest forgetting strong typing for this enum but still solve the scoping problem with:

class RPCErrorCode { public: enum { ... } }

Why not just make JSONRPCError accept a RPCErrorCode argument?

@luke-jr Yes, that's my plan. I'll push those changes today hopefully :-)

@luke-jr there would still be a few code == static_cast<int>(RPCErrorCode:: ...). But I agree it's not that many.

However changing JSONRPCError is kicking the can down to Pair where the conversion to int has to be done, or down to UniValue. It just seems to me like RPCErrorCode implicitly should be treated like an int since it will almost always be converted into one.

@luke-jr @ArielLorenzo-Luaces Signature now changed:

-UniValue JSONRPCError(int code, const std::string& message) +UniValue JSONRPCError(RPCErrorCode code, const std::string& message)

Only three static_cast<int>(RPCErrorCode) remain, and since they are explicit they should not cause any surprises (as opposed to implicit conversions).

I still think that RPCErrorCode and HTTPStatusCode are intrinsically different than the other enumerators, since they are sent over HTTP. They are also the only two that have explicit values assigned. They should be treated as integers always and they should require no conversion.

I would have suggested to hide conversion in encode/decode functions. Then the conversion would only have to be done once after receiving an http status. But then I see response.status >= 400 in bitcoin-cli.cpp. This suggests that HTTP status codes are not treated like enumerators and should not be compared to enumerators either.

RPCErrorCode and HTTPStatusCode should be treated more like macros instead.
Strong typing them would case unnecessary complexity due to the casting. But that would only be a problem if they begin to be used much more often, which is speculation on my part. So these changes are fine for now.

practicalswift · 2017-07-15T17:38:22Z

Rebased!

practicalswift · 2017-07-18T04:04:54Z

Rebased again! :-)

practicalswift · 2017-08-01T09:23:40Z

Rebased! :-)

practicalswift · 2018-03-27T11:03:33Z

@laanwj Would it be possible to get a 0.17.0 milestone for this PR? :-)

laanwj · 2018-03-27T14:30:44Z

Probably better to just merge this as there seems agreement to do it...
utACK 1f45e21

…ass") 1f45e21 scripted-diff: Convert 11 enums into scoped enums (C++11) (practicalswift) Pull request description: Rationale (from Bjarne Stroustrup's ["C++11 FAQ"](http://www.stroustrup.com/C++11FAQ.html#enum)): > > The enum classes ("new enums", "strong enums") address three problems with traditional C++ enumerations: > > * conventional enums implicitly convert to int, causing errors when someone does not want an enumeration to act as an integer. > * conventional enums export their enumerators to the surrounding scope, causing name clashes. > * the underlying type of an enum cannot be specified, causing confusion, compatibility problems, and makes forward declaration impossible. > > The new enums are "enum class" because they combine aspects of traditional enumerations (names values) with aspects of classes (scoped members and absence of conversions). Tree-SHA512: 9656e1cf4c3cabd4378c7a38d0c2eaf79e4a54d204a3c5762330840e55ee7e141e188a3efb2b4daf0ef3110bbaff80d8b9253abf2a9b015cdc4d60b49ac2b914

…ons ("enum class") 0fee2b4 doc: Add note about our preference for scoped enumerations ("enum class") (practicalswift) Pull request description: Add note about our preference for scoped enumerations (`enum class`). Context: #10742 Tree-SHA512: 0ab3465c2b734240cb38a05c2f6e75f1af54207a0f1a2e8115e7b367fd37e8966a2fc0240c6d4c2c66b6677b5f367eda4f4b783bbaa419777336c17f04adff06

…with some compilers) 43f3dec Remove enum specifier (to avoid re-declare scoped enum as unscoped) (donaloconnor) Pull request description: MSVC fails to compile with the changes made in #10742 The problem is enum types were changed to scoped (`enum class`) but in some places `enum` as an unscoped is used. This is a very simple fix and I've tested it. Edit: Had to remove enum altogether - `enum class` doesn't compile on clang. Tree-SHA512: 13e21666243585a133c74c81249a1fa4098d6b7aa3cda06be871fa017c0ad9bb7b0725f801160b9d31678448d668718197941fd84702ebdef15128c27d92cd70

References: - bitcoin/bitcoin#11423 - bitcoin/bitcoin#12600 - bitcoin/bitcoin#12082 Trivial References: - bitcoin/bitcoin#12393 - bitcoin/bitcoin#6539 - bitcoin/bitcoin#10742 - bitcoin/bitcoin@ecb11f5

…oin#10742) -BEGIN VERIFY SCRIPT- sed -i 's/enum DBErrors/enum class DBErrors/g' src/wallet/walletdb.h git grep -l DB_ | xargs sed -i 's/DB_\(LOAD_OK\|CORRUPT\|NONCRITICAL_ERROR\|TOO_NEW\|LOAD_FAIL\|NEED_REWRITE\)/DBErrors::\1/g' sed -i 's/^ DBErrors::/ /g' src/wallet/walletdb.h sed -i 's/enum VerifyResult/enum class VerifyResult/g' src/wallet/db.h sed -i 's/\(VERIFY_OK\|RECOVER_OK\|RECOVER_FAIL\)/VerifyResult::\1/g' src/wallet/db.cpp sed -i 's/enum ThresholdState/enum class ThresholdState/g' src/versionbits.h git grep -l THRESHOLD_ | xargs sed -i 's/THRESHOLD_\(DEFINED\|STARTED\|LOCKED_IN\|ACTIVE\|FAILED\)/ThresholdState::\1/g' sed -i 's/^ ThresholdState::/ /g' src/versionbits.h sed -i 's/enum SigVersion/enum class SigVersion/g' src/script/interpreter.h git grep -l SIGVERSION_ | xargs sed -i 's/SIGVERSION_\(BASE\|WITNESS_V0\)/SigVersion::\1/g' sed -i 's/^ SigVersion::/ /g' src/script/interpreter.h sed -i 's/enum RetFormat {/enum class RetFormat {/g' src/rest.cpp sed -i 's/RF_\(UNDEF\|BINARY\|HEX\|JSON\)/RetFormat::\1/g' src/rest.cpp sed -i 's/^ RetFormat::/ /g' src/rest.cpp sed -i 's/enum HelpMessageMode {/enum class HelpMessageMode {/g' src/init.h git grep -l HMM_ | xargs sed -i 's/HMM_BITCOIN/HelpMessageMode::BITCOIN/g' sed -i 's/^ HelpMessageMode::/ /g' src/init.h sed -i 's/enum FeeEstimateHorizon/enum class FeeEstimateHorizon/g' src/policy/fees.h sed -i 's/enum BlockSource {/enum class BlockSource {/g' src/qt/clientmodel.h git grep -l BLOCK_SOURCE_ | xargs sed -i 's/BLOCK_SOURCE_\(NONE\|REINDEX\|DISK\|NETWORK\)/BlockSource::\1/g' sed -i 's/^ BlockSource::/ /g' src/qt/clientmodel.h sed -i 's/enum FlushStateMode {/enum class FlushStateMode {/g' src/validation.cpp sed -i 's/FLUSH_STATE_\(NONE\|IF_NEEDED\|PERIODIC\|ALWAYS\)/FlushStateMode::\1/g' src/validation.cpp sed -i 's/^ FlushStateMode::/ /g' src/validation.cpp -END VERIFY SCRIPT- Signed-off-by: pasta <pasta@dashboost.org>

…issues with some compilers) 43f3dec Remove enum specifier (to avoid re-declare scoped enum as unscoped) (donaloconnor) Pull request description: MSVC fails to compile with the changes made in bitcoin#10742 The problem is enum types were changed to scoped (`enum class`) but in some places `enum` as an unscoped is used. This is a very simple fix and I've tested it. Edit: Had to remove enum altogether - `enum class` doesn't compile on clang. Tree-SHA512: 13e21666243585a133c74c81249a1fa4098d6b7aa3cda06be871fa017c0ad9bb7b0725f801160b9d31678448d668718197941fd84702ebdef15128c27d92cd70

…umerations ("enum class") 0fee2b4 doc: Add note about our preference for scoped enumerations ("enum class") (practicalswift) Pull request description: Add note about our preference for scoped enumerations (`enum class`). Context: bitcoin#10742 Tree-SHA512: 0ab3465c2b734240cb38a05c2f6e75f1af54207a0f1a2e8115e7b367fd37e8966a2fc0240c6d4c2c66b6677b5f367eda4f4b783bbaa419777336c17f04adff06

…oin#10742) -BEGIN VERIFY SCRIPT- sed -i 's/enum DBErrors/enum class DBErrors/g' src/wallet/walletdb.h git grep -l DB_ | xargs sed -i 's/DB_\(LOAD_OK\|CORRUPT\|NONCRITICAL_ERROR\|TOO_NEW\|LOAD_FAIL\|NEED_REWRITE\)/DBErrors::\1/g' sed -i 's/^ DBErrors::/ /g' src/wallet/walletdb.h sed -i 's/enum VerifyResult/enum class VerifyResult/g' src/wallet/db.h sed -i 's/\(VERIFY_OK\|RECOVER_OK\|RECOVER_FAIL\)/VerifyResult::\1/g' src/wallet/db.cpp sed -i 's/enum ThresholdState/enum class ThresholdState/g' src/versionbits.h git grep -l THRESHOLD_ | xargs sed -i 's/THRESHOLD_\(DEFINED\|STARTED\|LOCKED_IN\|ACTIVE\|FAILED\)/ThresholdState::\1/g' sed -i 's/^ ThresholdState::/ /g' src/versionbits.h sed -i 's/enum SigVersion/enum class SigVersion/g' src/script/interpreter.h git grep -l SIGVERSION_ | xargs sed -i 's/SIGVERSION_\(BASE\|WITNESS_V0\)/SigVersion::\1/g' sed -i 's/^ SigVersion::/ /g' src/script/interpreter.h sed -i 's/enum RetFormat {/enum class RetFormat {/g' src/rest.cpp sed -i 's/RF_\(UNDEF\|BINARY\|HEX\|JSON\)/RetFormat::\1/g' src/rest.cpp sed -i 's/^ RetFormat::/ /g' src/rest.cpp sed -i 's/enum HelpMessageMode {/enum class HelpMessageMode {/g' src/init.h git grep -l HMM_ | xargs sed -i 's/HMM_BITCOIN/HelpMessageMode::BITCOIN/g' sed -i 's/^ HelpMessageMode::/ /g' src/init.h sed -i 's/enum FeeEstimateHorizon/enum class FeeEstimateHorizon/g' src/policy/fees.h sed -i 's/enum BlockSource {/enum class BlockSource {/g' src/qt/clientmodel.h git grep -l BLOCK_SOURCE_ | xargs sed -i 's/BLOCK_SOURCE_\(NONE\|REINDEX\|DISK\|NETWORK\)/BlockSource::\1/g' sed -i 's/^ BlockSource::/ /g' src/qt/clientmodel.h sed -i 's/enum FlushStateMode {/enum class FlushStateMode {/g' src/validation.cpp sed -i 's/FLUSH_STATE_\(NONE\|IF_NEEDED\|PERIODIC\|ALWAYS\)/FlushStateMode::\1/g' src/validation.cpp sed -i 's/^ FlushStateMode::/ /g' src/validation.cpp -END VERIFY SCRIPT- Signed-off-by: pasta <pasta@dashboost.org>

…issues with some compilers) 43f3dec Remove enum specifier (to avoid re-declare scoped enum as unscoped) (donaloconnor) Pull request description: MSVC fails to compile with the changes made in bitcoin#10742 The problem is enum types were changed to scoped (`enum class`) but in some places `enum` as an unscoped is used. This is a very simple fix and I've tested it. Edit: Had to remove enum altogether - `enum class` doesn't compile on clang. Tree-SHA512: 13e21666243585a133c74c81249a1fa4098d6b7aa3cda06be871fa017c0ad9bb7b0725f801160b9d31678448d668718197941fd84702ebdef15128c27d92cd70

practicalswift force-pushed the scoped-enums branch 2 times, most recently from 6c9c5a7 to 4602120 Compare July 4, 2017 17:02

fanquake added the Refactoring label Jul 5, 2017

ArielLorenzo-Luaces approved these changes Jul 5, 2017

View reviewed changes

practicalswift force-pushed the scoped-enums branch 5 times, most recently from 25333b1 to 1840ee7 Compare July 6, 2017 21:32

practicalswift force-pushed the scoped-enums branch 2 times, most recently from e5a7dc6 to 5255dc7 Compare July 15, 2017 15:51

practicalswift force-pushed the scoped-enums branch 3 times, most recently from cd78823 to 28cd41d Compare July 18, 2017 04:04

practicalswift force-pushed the scoped-enums branch 6 times, most recently from 3091ec1 to e4dd710 Compare July 24, 2017 14:04

practicalswift force-pushed the scoped-enums branch 5 times, most recently from 040b561 to cd8b270 Compare August 1, 2017 07:57

practicalswift force-pushed the scoped-enums branch 2 times, most recently from 71ede09 to 1dcbb3c Compare August 3, 2017 12:38

laanwj merged commit 1f45e21 into bitcoin:master Mar 27, 2018

practicalswift mentioned this pull request Mar 27, 2018

doc: Add note about our preference for scoped enumerations ("enum class") #12800

Merged

ryanofsky mentioned this pull request Mar 27, 2018

scripted-diff: Rename wallet database classes #11851

Merged

donaloconnor mentioned this pull request May 6, 2018

Fix re-declared scoped enum as unscoped (Causes issues with some compilers) #13180

Merged

practicalswift deleted the scoped-enums branch April 10, 2021 19:33

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

scripted-diff: Use scoped enumerations (C++11, "enum class") #10742

scripted-diff: Use scoped enumerations (C++11, "enum class") #10742

Uh oh!

practicalswift commented Jul 4, 2017

Uh oh!

ArielLorenzo-Luaces Jul 5, 2017

Uh oh!

luke-jr Jul 5, 2017

Uh oh!

practicalswift Jul 5, 2017

Uh oh!

ArielLorenzo-Luaces Jul 5, 2017

Uh oh!

practicalswift Jul 5, 2017 •

edited

Loading

Uh oh!

ArielLorenzo-Luaces Jul 5, 2017

Uh oh!

practicalswift commented Jul 15, 2017

Uh oh!

practicalswift commented Jul 18, 2017

Uh oh!

practicalswift commented Aug 1, 2017

Uh oh!

practicalswift commented Mar 27, 2018

Uh oh!

laanwj commented Mar 27, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

scripted-diff: Use scoped enumerations (C++11, "enum class") #10742

scripted-diff: Use scoped enumerations (C++11, "enum class") #10742

Uh oh!

Conversation

practicalswift commented Jul 4, 2017

Uh oh!

ArielLorenzo-Luaces Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

luke-jr Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

ArielLorenzo-Luaces Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArielLorenzo-Luaces Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jul 15, 2017

Uh oh!

practicalswift commented Jul 18, 2017

Uh oh!

practicalswift commented Aug 1, 2017

Uh oh!

practicalswift commented Mar 27, 2018

Uh oh!

laanwj commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

practicalswift Jul 5, 2017 •

edited

Loading

laanwj commented Mar 27, 2018 •

edited

Loading