Skip to content

Use non-throwing type-safe ChainType where possible #14309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
maflcko wants to merge 4 commits into from
Closed

Use non-throwing type-safe ChainType where possible #14309

maflcko wants to merge 4 commits into from

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Sep 24, 2018 •
edited
Loading

Currently we use string constants to select different chain rules (e.g "main" or "test"). The obvious downsides are that any function that accepts such a string needs to check if it is one of the known ones and return a runtime error when not found.

This check can be done at compile time by using a switch statement on an enum class.

To not refactor all the existing code, this first introduces the type-safe enum class ChainType and then runs a scripted diff on all instances where we can benefit from the new class. I intend to remove the throwing and now deprecated methods that pass around the string in a future patch.

@practicalswift
Copy link
Contributor

practicalswift commented Sep 24, 2018

Concept ACK

Nice!

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 24, 2018 •
edited
Loading

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 was referenced Sep 24, 2018
Merged
Closed
This was referenced Sep 24, 2018
Closed
Merged
@maflcko maflcko force-pushed the Mf1809-chainType branch 3 times, most recently from 3e75144 to 471c5de Compare September 26, 2018 03:46
@DrahtBot DrahtBot mentioned this pull request Sep 26, 2018
Merged
@maflcko maflcko force-pushed the Mf1809-chainType branch 3 times, most recently from fa80025 to fadec8b Compare September 26, 2018 12:16
@maflcko maflcko changed the title scripted-diff: Use non-throwing type-safe ChainType where possible Use non-throwing type-safe ChainType where possible Sep 26, 2018
@maflcko maflcko force-pushed the Mf1809-chainType branch 2 times, most recently from a892601 to fab0c08 Compare September 26, 2018 12:47
MarcoFalke added 2 commits September 26, 2018 09:07
chainparams: Add type safe enum class ChainType
d341856
scripted-diff: Use non-throwing type-safe ChainType where possible
731037e 
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/(Create(Base)?ChainParams)\(C(Base)?ChainParams::/\1(ChainType::/g' $(git grep -l --extended-regexp 'Create(Base)?ChainParams')
sed -i --regexp-extended -e 's/((S|s)elect(Base)?Params)\(C(Base)?ChainParams::/\1(ChainType::/g' $(git grep -l --extended-regexp 'elect(Base)?Params')
-END VERIFY SCRIPT-
@maflcko maflcko force-pushed the Mf1809-chainType branch 3 times, most recently from fa5cf90 to fa83f32 Compare September 26, 2018 23:36
@practicalswift
Copy link
Contributor

practicalswift commented Sep 27, 2018 •
edited
Loading

What about adding the NODISCARD/[[nodiscard]] attribute to bool ParseChainType(const std::string& str, ChainType& chain) to guarantee that all future callers make use of the return code?

See #13815 for NODISCARD which enables [[nodiscard]] (or equivalent) without having to wait for C++17.

This was referenced Sep 27, 2018
Merged
Draft
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 28, 2018

Coverage Change (pull 14309) Reference (master)
Lines +0.0423 % 87.0361 %
Functions +0.0788 % 84.1130 %
Branches +0.0236 % 51.5451 %

@DrahtBot DrahtBot mentioned this pull request Sep 28, 2018
Merged
@DrahtBot DrahtBot mentioned this pull request Sep 28, 2018
Merged
@DrahtBot DrahtBot mentioned this pull request Oct 9, 2018
Merged
This was referenced Oct 20, 2018
Merged
Closed
Closed
@jtimon
Copy link
Contributor

jtimon commented Oct 20, 2018

why not a macro or a simple const string in chainbaseparams.h ?
I think the problem I see is the string cannot be used in certain place now (ie chainparams.cpp for setting strNetworkID and in qt/networkstyle.cpp for setting network_styles) only comes from the fact that the strings are not constants but static attributes in the class.

The errors thrown are only thrown during initialization and some of them would disappear with #8994 I'm not sure this adds much value.

@maflcko
Copy link
Member Author

maflcko commented Oct 21, 2018

Closing for now, but I think in the longer term we should get rid of exceptions (especially during initialization, where we currently use a string to explicitly pass up errors).

@maflcko maflcko closed this Oct 21, 2018
@maflcko maflcko deleted the Mf1809-chainType branch October 21, 2018 01:50
@maflcko maflcko mentioned this pull request Feb 1, 2021
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maflcko @practicalswift @DrahtBot @jtimon