Skip to content

refactor: Use ChainType enum exhaustively #27611

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
Merged
merged 1 commit into from
May 10, 2023
Merged

refactor: Use ChainType enum exhaustively #27611

merged 1 commit into from
May 10, 2023

Conversation

TheCharlatan
Copy link
Contributor

This is a follow up of #27491, more concretely #27491 (comment), for not using default cases (as per the style guide), and #27491 (comment) and #27491 (comment) for avoiding dead code.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 10, 2023 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake
Concept ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@maflcko
Copy link
Member

maflcko commented May 10, 2023

Can remove (follow-up) from the title. Also, the docstring of the changed methods in the header is still wrong

@TheCharlatan
Copy link
Contributor Author

Re #27611 (comment)

Also, the docstring of the changed methods in the header is still wrong

Do you mean these TheCharlatan@0e7abdc ?

@TheCharlatan TheCharlatan changed the title refactor(follow-up): Use ChainType enum exhaustively refactor: Use ChainType enum exhaustively May 10, 2023
@maflcko
Copy link
Member

maflcko commented May 10, 2023

I mean the ones of the touched methods Create*Params(), which are documented to throw the wrong exception type. Now that you changed to assert, there is no exception at all.

@maflcko
Copy link
Member

maflcko commented May 10, 2023

Example:

diff --git a/src/chainparams.h b/src/chainparams.h
index 6a65f40f80..23b272cc41 100644
--- a/src/chainparams.h
+++ b/src/chainparams.h
@@ -26,7 +26,6 @@ class ArgsManager;
 /**
  * Creates and returns a std::unique_ptr<CChainParams> of the chosen chain.
  * @returns a CChainParams* of the chosen chain.
- * @throws a std::runtime_error if the chain is not supported.
  */
 std::unique_ptr<const CChainParams> CreateChainParams(const ArgsManager& args, const ChainType chain);
 
@@ -38,7 +37,6 @@ const CChainParams &Params();
 
 /**
  * Sets the params returned by Params() to those for the given chain name.
- * @throws std::runtime_error when the chain is not supported.
  */
 void SelectParams(const ChainType chain);

@TheCharlatan
Copy link
Contributor Author

Updated d0767a9 -> ef39bdf (followUp27491_0 -> followUp27491_1, compare)

  • Addressed @MarcoFalke's comment, removing exception docstring.
  • Added scripted-diff commit s/chain\ name/chain\ type/.

maflcko
maflcko approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm, found another nit, feel free to ignore

maflcko
maflcko reviewed May 10, 2023
View reviewed changes
@TheCharlatan
refactor: Use ChainType enum exhaustively
e230887 
This is a follow up of bitcoin#27491,
more concretely
bitcoin#27491 (comment),
for not using default cases (as per the style guide), and
bitcoin#27491 (comment) and
bitcoin#27491 (comment) for
avoiding dead code.

Also change chain name to chain type in docstrings
@TheCharlatan
Copy link
Contributor Author

Updated ef39bdf -> e230887 (followUp27491_1 -> followUp27491_2, compare)

  • Addressed @MarcoFalke's comment, but kept the first line of the docstring intact.
  • Squashed scripted-diff commit

maflcko
maflcko approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm

@fanquake fanquake mentioned this pull request May 10, 2023
Merged
fanquake
fanquake approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK e230887 - deals with almost all follow up comments out of #27491.

@hebasto
Copy link
Member

hebasto commented May 10, 2023

Concept ACK.

@fanquake fanquake merged commit 104eed1 into bitcoin:master May 10, 2023
hebasto
hebasto reviewed May 10, 2023
View reviewed changes
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.

Post-merge ACK e230887.

src/bitcoin-cli.cpp
return "";
}
assert(false);
Copy link
Member

Choose a reason for hiding this comment

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

Our Developer Notes also suggest a comment // no default case, so the compiler can warn about missing cases.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 10, 2023
@fanquake
Merge bitcoin#27611: refactor: Use ChainType enum exhaustively
bc01b73 
e230887 refactor: Use ChainType enum exhaustively (TheCharlatan)

Pull request description:

  This is a follow up of bitcoin#27491, more concretely bitcoin#27491 (comment), for not using default cases (as per the style guide), and bitcoin#27491 (comment) and bitcoin#27491 (comment) for avoiding dead code.

ACKs for top commit:
  fanquake:
    ACK e230887 - deals with almost all follow up comments out of bitcoin#27491.

Tree-SHA512: 1794190b03b91d3ca349a4da08e9610dbb3432983eee7cb21ecc758d1d7d710560c97661de14cdf493c28c00ebe8977511b4696055c0940e7f815b622dbacd16
@joostjager joostjager mentioned this pull request May 15, 2023
Merged
@bitcoin bitcoin locked and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto left review comments

@fanquake fanquake fanquake approved these changes

+1 more reviewer

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TheCharlatan @DrahtBot @maflcko @hebasto @fanquake