refactor: Use ChainType enum exhaustively

TheCharlatan commented at 6:19 am on May 10, 2023: contributor

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

DrahtBot commented at 6:19 am on May 10, 2023: contributor

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 commented at 6:27 am on May 10, 2023: member

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

TheCharlatan commented at 6:51 am on May 10, 2023: contributor

Re #27611 (comment)

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

Do you mean these https://github.com/TheCharlatan/bitcoin/commit/0e7abdcb589ac2d891abce0b1cd9babbe6d0d39e ?

TheCharlatan renamed this:
~~refactor(follow-up): Use ChainType enum exhaustively~~
refactor: Use ChainType enum exhaustively
on May 10, 2023

DrahtBot added the label Refactoring on May 10, 2023

maflcko commented at 7:02 am on May 10, 2023: member

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 commented at 7:03 am on May 10, 2023: member

Example:

 0diff --git a/src/chainparams.h b/src/chainparams.h
 1index 6a65f40f80..23b272cc41 100644
 2--- a/src/chainparams.h
 3+++ b/src/chainparams.h
 4@@ -26,7 +26,6 @@ class ArgsManager;
 5 /**
 6  * Creates and returns a std::unique_ptr<CChainParams> of the chosen chain.
 7  * [@returns](/bitcoin-bitcoin/contributor/returns/) a CChainParams* of the chosen chain.
 8- * [@throws](/bitcoin-bitcoin/contributor/throws/) a std::runtime_error if the chain is not supported.
 9  */
10 std::unique_ptr<const CChainParams> CreateChainParams(const ArgsManager& args, const ChainType chain);
11 
12@@ -38,7 +37,6 @@ const CChainParams &Params();
13 
14 /**
15  * Sets the params returned by Params() to those for the given chain name.
16- * [@throws](/bitcoin-bitcoin/contributor/throws/) std::runtime_error when the chain is not supported.
17  */
18 void SelectParams(const ChainType chain);
19

TheCharlatan force-pushed on May 10, 2023

TheCharlatan commented at 8:11 am on May 10, 2023: contributor

Updated d0767a9e0af367cbcc4ff7fce3758ece7d62861f -> ef39bdf0f709db1b3bfa07adddce7cccbd774109 (followUp27491_0 -> followUp27491_1, compare)

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

in src/chainparamsbase.h:38 in ef39bdf0f7 outdated

35@@ -36,7 +36,6 @@ class CBaseChainParams
36 /**
37  * Creates and returns a std::unique_ptr<CBaseChainParams> of the chosen chain.
38  * @returns a CBaseChainParams* of the chosen chain.

maflcko commented at 8:28 am on May 10, 2023:

The return type is wrong, too. So maybe remove this line as well, as it is redundant with the line above it. Also, in the line above, can remove the return type, as it is redundant with the C++ code?

in src/chainparams.h:28 in ef39bdf0f7 outdated

25@@ -26,7 +26,6 @@ class ArgsManager;
26 /**
27  * Creates and returns a std::unique_ptr<CChainParams> of the chosen chain.
28  * @returns a CChainParams* of the chosen chain.

maflcko commented at 8:29 am on May 10, 2023:

Same

maflcko approved

maflcko commented at 8:29 am on May 10, 2023: member

lgtm, found another nit, feel free to ignore

refactor: Use ChainType enum exhaustively

This is a follow up of https://github.com/bitcoin/bitcoin/pull/27491,
more concretely
https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847896,
for not using default cases (as per the style guide), and
https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188852707 and
https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188851857 for
avoiding dead code.

Also change chain name to chain type in docstrings

e23088707b

in src/chainparams.h:38 in ef39bdf0f7 outdated

35@@ -36,7 +36,7 @@ std::unique_ptr<const CChainParams> CreateChainParams(const ArgsManager& args, c
36 const CChainParams &Params();
37 
38 /**
39- * Sets the params returned by Params() to those for the given chain name.
40+ * Sets the params returned by Params() to those for the given chain type.

maflcko commented at 8:31 am on May 10, 2023:

I think the scripted diff can be dropped, given that it fails? Maybe include this diff in the previous commit?

TheCharlatan force-pushed on May 10, 2023

DrahtBot added the label CI failed on May 10, 2023

TheCharlatan commented at 9:04 am on May 10, 2023: contributor

Updated ef39bdf0f709db1b3bfa07adddce7cccbd774109 -> e23088707be2c3bf247f4b777290c8e401db48cb (followUp27491_1 -> followUp27491_2, compare)

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

maflcko approved

maflcko commented at 9:13 am on May 10, 2023: member

lgtm

DrahtBot removed the label CI failed on May 10, 2023

fanquake approved

fanquake commented at 11:10 am on May 10, 2023: member

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

hebasto commented at 11:19 am on May 10, 2023: member

Concept ACK.

fanquake merged this on May 10, 2023

fanquake closed this on May 10, 2023

in src/bitcoin-cli.cpp:440 in e23088707b

433@@ -434,9 +434,10 @@ class NetinfoRequestHandler : public BaseRequestHandler
434             return " signet";
435         case ChainType::REGTEST:
436             return " regtest";
437-        default:
438+        case ChainType::MAIN:
439             return "";
440         }
441+        assert(false);

hebasto commented at 11:36 am on May 10, 2023:

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

hebasto commented at 11:37 am on May 10, 2023: member

Post-merge ACK e23088707be2c3bf247f4b777290c8e401db48cb.

sidhujag referenced this in commit bc01b73390 on May 10, 2023

bitcoin locked this on May 9, 2024

refactor: Use ChainType enum exhaustively #27611

Reviews