Refactor: One CBaseChainParams should be enough

jtimon commented at 2:48 am on January 9, 2018: contributor

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

fanquake added the label Refactoring on Jan 9, 2018

in src/chainparamsbase.h:29 in a26c919e47 outdated

25@@ -24,9 +26,12 @@ class CBaseChainParams
26     const std::string& DataDir() const { return strDataDir; }
27     int RPCPort() const { return nRPCPort; }
28 
29-protected:
30-    CBaseChainParams() {}
31+    CBaseChainParams(const std::string& strDataDir, int nRPCPort) {

TheBlueMatt commented at 9:39 pm on January 9, 2018:

codestyle: just use data_dir and rpc_port.

MarcoFalke commented at 7:03 pm on January 10, 2018:

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

in src/chainparamsbase.h:34 in a26c919e47 outdated

31+    CBaseChainParams(const std::string& strDataDir, int nRPCPort) {
32+        this->strDataDir = strDataDir;
33+        this->nRPCPort = nRPCPort;
34+    }
35 
36+protected:

jimpo commented at 6:33 pm on January 10, 2018:

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

in src/chainparamsbase.h:19 in a26c919e47 outdated

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

jimpo commented at 6:34 pm on January 10, 2018:

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

jimpo commented at 10:25 pm on January 31, 2018:

@jtimon ^

in src/chainparamsbase.cpp:38 in a26c919e47 outdated

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

jimpo commented at 6:35 pm on January 10, 2018:

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

jtimon commented at 8:14 pm on January 11, 2018:

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

ryanofsky commented at 8:35 pm on January 11, 2018:

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

https://github.com/bitcoin/bitcoin/blob/0910cbe4ef31eb95fd76c7c2f820419fe64a3150/src/util.h#L336-L338

jimpo commented at 6:36 pm on January 10, 2018: contributor

Concept ACK.

jtimon force-pushed on Jan 11, 2018

jtimon commented at 8:09 pm on January 11, 2018: contributor

Fixed nits

promag commented at 8:54 pm on January 11, 2018: member

utACK after #12128 (review) is addressed.

jtimon force-pushed on Jan 11, 2018

jtimon commented at 10:20 pm on January 11, 2018: contributor

Fixed last nit (using util function MakeUnique).

jtimon force-pushed on Jan 12, 2018

in src/chainparamsbase.h:34 in 277b4a994c outdated

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

ajtowns commented at 1:55 am on February 1, 2018:

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:

0+    const int nRPCPort;
1+    const std::string strDataDir; 
2
3+    CBaseChainParams() = delete;
4+    CBaseChainParams(const CBaseChainParams&) = delete;
5+    CBaseChainParams& operator=(const CBaseChainParams&) = delete;

jtimon commented at 9:27 pm on February 1, 2018:

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.

ajtowns commented at 3:29 am on February 2, 2018:

(= 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:

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

gcc:

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

clang:

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

jtimon commented at 3:10 am on February 6, 2018:

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

0CBaseChainParams() = 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.

ajtowns commented at 1:57 am on February 1, 2018: member

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)

Refactor: One CBaseChainParams should be enough 1687cb4a87

jtimon force-pushed on Feb 8, 2018

jtimon commented at 9:09 pm on February 8, 2018: contributor

Fixed nit (moved to deleted constructor).

MarcoFalke commented at 9:24 pm on February 8, 2018: member

utACK 1687cb4a879ee6ec8c5abf20a2c92f5fe201a66b

jimpo commented at 11:39 pm on February 8, 2018: contributor

utACK 1687cb4

promag commented at 0:04 am on February 9, 2018: member

utACK 1687cb4. Nice change.

ajtowns commented at 3:18 am on February 9, 2018: member

utACK 1687cb4a879ee6ec8c5abf20a2c92f5fe201a66b

laanwj merged this on Feb 10, 2018

laanwj closed this on Feb 10, 2018

laanwj referenced this in commit 948c29cc0d on Feb 10, 2018

jtimon deleted the branch on Feb 11, 2018

jtimon referenced this in commit a659102d3f on Feb 19, 2018

Mengerian referenced this in commit 784be9ae12 on Aug 6, 2019

jonspock referenced this in commit 18e427e097 on Sep 8, 2019

jonspock referenced this in commit 7d07f465f5 on Sep 9, 2019

proteanx referenced this in commit 2a41c08588 on Sep 10, 2019

xdustinface referenced this in commit 53e57aced2 on Dec 10, 2020

xdustinface referenced this in commit 0e65473daf on Dec 10, 2020

furszy referenced this in commit 6e98b237d6 on May 26, 2021

DrahtBot locked this on Sep 8, 2021

Refactor: One CBaseChainParams should be enough #12128