Refactor: One CBaseChainParams should be enough #12128

jtimon · 2018-01-09T02:48:55Z

There's no need for class hierarchy with CBaseChainParams, it is just a struct with 2 fields.
This starts as a +10-43 diff

TheBlueMatt · 2018-01-09T21:39:16Z

src/chainparamsbase.h

@@ -24,9 +26,12 @@ class CBaseChainParams
    const std::string& DataDir() const { return strDataDir; }
    int RPCPort() const { return nRPCPort; }

-protected:
-    CBaseChainParams() {}
+    CBaseChainParams(const std::string& strDataDir, int nRPCPort) {


codestyle: just use data_dir and rpc_port.

and then nRPCPort(rpc_port), strDataDir(data_dir) {} constructor initializer list.

jimpo

Concept ACK.

jimpo · 2018-01-10T18:33:17Z

src/chainparamsbase.h


+protected:


Now that subclasses are removed, these can be private I think.

jimpo · 2018-01-10T18:34:49Z

src/chainparamsbase.h

@@ -15,6 +15,8 @@
 */
 class CBaseChainParams
 {
+    /** Default constructor is private to force the use of the parametrized one */
+    CBaseChainParams() {}


Maybe use C++11 deleted function syntax? CBaseChainParams() = delete.

jimpo · 2018-01-10T18:35:47Z

src/chainparamsbase.cpp

@@ -73,11 +35,11 @@ const CBaseChainParams& BaseParams()
 std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const std::string& chain)
 {
    if (chain == CBaseChainParams::MAIN)
-        return std::unique_ptr<CBaseChainParams>(new CBaseMainParams());
+        return std::unique_ptr<CBaseChainParams>(new CBaseChainParams("", 8332));


nit: Could use MakeUnique<CBaseChainParams>("", 8332)

I tried, but it seems it's a C++14 feature, not C++11. :(

c++14 provides make_unique, but MakeUnique is just a function in util:

bitcoin/src/util.h

Lines 336 to 338 in 0910cbe

//! Substitute for C++14 std::make_unique.

template <typename T, typename... Args>

std::unique_ptr<T> MakeUnique(Args&&... args)

jtimon · 2018-01-11T20:09:29Z

Fixed nits

promag · 2018-01-11T20:54:19Z

utACK after #12128 (comment) is addressed.

jtimon · 2018-01-11T22:20:19Z

Fixed last nit (using util function MakeUnique).

ajtowns

utACK 277b4a994c83f131024ba8e24e3dc247078a0c82 but I agree with @jimpo's nit that the default constructor should just be deleted. (Could delete the copy constructors too, but that doesn't achieve a lot)

ajtowns · 2018-02-01T01:55:04Z

src/chainparamsbase.h

    int nRPCPort;
    std::string strDataDir;
+
+    /** Default constructor is private to force the use of the parametrized one */
+    CBaseChainParams() {}


Having a non-default constructor already prevents automatic declaration of the default constructor; but "CBaseChainParams() = delete;" seems clearer still.

Also, since nRPCPort and strDataDir are set via initializer list, and aren't fiddled with later (they're private and all the member functions are marked const), they could be marked "const" too.

Tested with these changes:

+ const int nRPCPort; + const std::string strDataDir; + CBaseChainParams() = delete; + CBaseChainParams(const CBaseChainParams&) = delete; + CBaseChainParams& operator=(const CBaseChainParams&) = delete;

Having a non-default constructor already prevents automatic declaration of the default constructor

Does it? I really think that's not the case, do you have any reference?

but "CBaseChainParams() = delete;" seems clearer still.

Perhaps. I've never seen this contruct but if people prefer this I'm happy to change it.

(= delete is new in C++11)

Reference: http://en.cppreference.com/w/cpp/language/default_constructor

If no user-declared constructors of any kind are provided for a class type (struct, class, or union), the compiler will always declare a default constructor as an inline public member of its class.

[[c++11:]] If some user-declared constructors are present, the user may still force the automatic generation of a default constructor by the compiler that would be implicitly-declared otherwise with the keyword default.

Example:

class X { public: int a; char b; X(int _a, char _b) : a(_a), b(_b) { } }; X x;

gcc:

test.cc:2:3: error: no matching function for call to ‘X::X()’ X x; ^ test.cc:1:34: note: candidate: X::X(int, char) test.cc:1:34: note: candidate expects 2 arguments, 0 provided test.cc:1:7: note: candidate: constexpr X::X(const X&) test.cc:1:7: note: candidate expects 1 argument, 0 provided test.cc:1:7: note: candidate: constexpr X::X(X&&) test.cc:1:7: note: candidate expects 1 argument, 0 provided

clang:

test.cc:2:3: error: no matching constructor for initialization of 'X' X x; ^ test.cc:1:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 0 were provided test.cc:1:7: note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 0 were provided test.cc:1:34: note: candidate constructor not viable: requires 2 arguments, but 0 were provided class X { public: int a; char b; X(int _a, char _b) : a(_a), b(_b) { } }; ^

Thinking more about this, I see how your interest in also prohibiting the copy constructor and the override of the assign operator may make sense in many cases, but I think in this case we're just fine prohibiting only the default contructor. Anything copied from an instance initialized with the parametrized contructor should be fine.
But thank you for teaching me newer c++ features, I love to be able to ditch that hackish design pattern.
I will

CBaseChainParams() = delete;

Actually the fact that an upgrade of the language supports what I was "simulating" gives me much more confidence on that part. Thank you again for stopping me from reinventing the wheel.

jtimon · 2018-02-08T21:09:05Z

Fixed nit (moved to deleted constructor).

maflcko · 2018-02-08T21:24:20Z

utACK 1687cb4

jimpo · 2018-02-08T23:39:33Z

utACK 1687cb4

promag · 2018-02-09T00:04:55Z

utACK 1687cb4. Nice change.

ajtowns · 2018-02-09T03:18:36Z

utACK 1687cb4

1687cb4 Refactor: One CBaseChainParams should be enough (Jorge Timón) Pull request description: There's no need for class hierarchy with CBaseChainParams, it is just a struct with 2 fields. This starts as a +10-43 diff Tree-SHA512: 0a7dd64ab785416550b541787c6083540e4962d76b6cffa806bb3593aec2daf1752dfe65ac5cd51b34ad5c31dd8292c422b483fdd2d37d0b7e68725498ed4c2d

Summary: 1687cb4 Refactor: One CBaseChainParams should be enough (Jorge Timón) Pull request description: There's no need for class hierarchy with CBaseChainParams, it is just a struct with 2 fields. This starts as a +10-43 diff Tree-SHA512: 0a7dd64ab785416550b541787c6083540e4962d76b6cffa806bb3593aec2daf1752dfe65ac5cd51b34ad5c31dd8292c422b483fdd2d37d0b7e68725498ed4c2d Backport of Core PR12128 bitcoin/bitcoin#12128 Test Plan: make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3804

Summary: 1687cb4 Refactor: One CBaseChainParams should be enough (Jorge Timón) Pull request description: There's no need for class hierarchy with CBaseChainParams, it is just a struct with 2 fields. This starts as a +10-43 diff Tree-SHA512: 0a7dd64ab785416550b541787c6083540e4962d76b6cffa806bb3593aec2daf1752dfe65ac5cd51b34ad5c31dd8292c422b483fdd2d37d0b7e68725498ed4c2d Backport of Core PR12128 bitcoin/bitcoin#12128 Test Plan: make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3804

1687cb4 Refactor: One CBaseChainParams should be enough (Jorge Timón) Pull request description: There's no need for class hierarchy with CBaseChainParams, it is just a struct with 2 fields. This starts as a +10-43 diff Tree-SHA512: 0a7dd64ab785416550b541787c6083540e4962d76b6cffa806bb3593aec2daf1752dfe65ac5cd51b34ad5c31dd8292c422b483fdd2d37d0b7e68725498ed4c2d

…ugh (#3865) 1687cb4 Refactor: One CBaseChainParams should be enough (Jorge Timón) Pull request description: There's no need for class hierarchy with CBaseChainParams, it is just a struct with 2 fields. This starts as a +10-43 diff Tree-SHA512: 0a7dd64ab785416550b541787c6083540e4962d76b6cffa806bb3593aec2daf1752dfe65ac5cd51b34ad5c31dd8292c422b483fdd2d37d0b7e68725498ed4c2d Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>

a58be04 Use a unique RPC port for regtest (Fuzzbawls) d9f1710 One CBaseChainParams should be enough (Fuzzbawls) Pull request description: Coming from bitcoin#12128 > There's no need for class hierarchy with CBaseChainParams, it is just a struct with 2 fields. > This starts as a +10-43 diff Since we already require c++14, can use `std::make_unique` directly. Note: I did not change the regtest RPC port here, but maybe this is the perfect opportunity to do so if we want it to not conflict with testnet's RPC port ACKs for top commit: random-zebra: utACK a58be04 furszy: utACK a58be04 Tree-SHA512: 2c4d70931e7228cec35113ed28732410efc6f0daca75eef7abc2f4337919fca170407635044ca4096a5af5a802280f414f5422b6d2607fbeca1e62f78386107d

jtimon mentioned this pull request Jan 9, 2018

Testchains: Introduce custom chain whose constructor... #8994

Closed

12 tasks

fanquake added the Refactoring label Jan 9, 2018

TheBlueMatt reviewed Jan 9, 2018

View reviewed changes

jimpo reviewed Jan 10, 2018

View reviewed changes

jtimon force-pushed the b16-baseparams-nohierarchy branch from a26c919 to 9b69179 Compare January 11, 2018 20:09

jtimon force-pushed the b16-baseparams-nohierarchy branch from 9b69179 to bbd0d1c Compare January 11, 2018 22:19

jtimon force-pushed the b16-baseparams-nohierarchy branch from bbd0d1c to 277b4a9 Compare January 12, 2018 23:11

ajtowns reviewed Feb 1, 2018

View reviewed changes

Refactor: One CBaseChainParams should be enough

1687cb4

jtimon force-pushed the b16-baseparams-nohierarchy branch from 277b4a9 to 1687cb4 Compare February 8, 2018 21:07

laanwj merged commit 1687cb4 into bitcoin:master Feb 10, 2018

jtimon deleted the b16-baseparams-nohierarchy branch February 11, 2018 20:15

Fuzzbawls mentioned this pull request May 25, 2021

[Refactor] One CBaseChainParams should be enough PIVX-Project/PIVX#2399

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

	//! Substitute for C++14 std::make_unique.
	template <typename T, typename... Args>
	std::unique_ptr<T> MakeUnique(Args&&... args)


		protected:

Refactor: One CBaseChainParams should be enough #12128

Refactor: One CBaseChainParams should be enough #12128

Uh oh!

Conversation

jtimon commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 11, 2018 • 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!

jtimon commented Jan 11, 2018

Uh oh!

promag commented Jan 11, 2018

Uh oh!

jtimon commented Jan 11, 2018

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon Feb 6, 2018 • 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!

jtimon commented Feb 8, 2018

Uh oh!

maflcko commented Feb 8, 2018

Uh oh!

jimpo commented Feb 8, 2018

Uh oh!

promag commented Feb 9, 2018

Uh oh!

ajtowns commented Feb 9, 2018

Uh oh!

Uh oh!

ryanofsky Jan 11, 2018 •

edited

Loading

jtimon Feb 6, 2018 •

edited

Loading