net: Use C++11 member initialization in protocol #19020

1. 
   MarcoFalke commented at 9:43 pm on May 19, 2020: member

   This change removes Init from the constructors and instead uses C++11 member initialization. This removes a bunch of boilerplate, makes the code easier to read. Also, C++11 member initialization avoids accidental uninitialized members.
2. in src/protocol.h:339 in eac91b0376 outdated

   336+    CAddress() : CService{} {};
   337+    explicit CAddress(CService ipIn, ServiceFlags nServicesIn) : CService{ipIn}, nServices{nServicesIn} {};
   338 
   339     SERIALIZE_METHODS(CAddress, obj)
   340     {
   341-        SER_READ(obj, obj.Init());
   

   ---

   ---

   

   sipa commented at 9:48 pm on May 19, 2020:

   This may be fine here, but it’s not obvious. When deserializing, the receiving object is expected to be overwritten completely. As nTime is only conditionally deserialized, the alternative would need a reset for that value.

   ---

   

   MarcoFalke commented at 12:37 pm on May 20, 2020:

   Thanks, fixed.
3. DrahtBot added the label Consensus on May 19, 2020
4. DrahtBot added the label P2P on May 19, 2020
5. DrahtBot added the label RPC/REST/ZMQ on May 19, 2020
6. DrahtBot added the label Tests on May 19, 2020
7. DrahtBot added the label TX fees and policy on May 19, 2020
8. DrahtBot added the label UTXO Db and Indexes on May 19, 2020
9. 
   DrahtBot commented at 7:20 am on May 20, 2020: member

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

   Conflicts

   No conflicts as of last run.

10. 
    practicalswift commented at 8:52 am on May 20, 2020: contributor

    Concept ACK

    AFAICT the suggested change is fully in line with with the recommendations in the C++ Core Guidelines (Stroustrup & Sutter):

    • C.41: A constructor should create a fully initialized object: “A constructor establishes the invariant for a class. A user of a class should be able to assume that a constructed object is usable.”
    • C.45: Don’t define a default constructor that only initializes data members; use in-class member initializers instead: “Using in-class member initializers lets the compiler generate the function for you. The compiler-generated function can be more efficient.”
    • C.48: Prefer in-class initializers to member initializers in constructors for constant initializers: “Makes it explicit that the same value is expected to be used in all constructors. Avoids repetition. Avoids maintenance problems. It leads to the shortest and most efficient code.”
    • C.49: Prefer initialization to assignment in constructors: “An initialization explicitly states that initialization, rather than assignment, is done and can be more elegant and efficient. Prevents “use before set” errors.”
    • ES.20: Always initialize an object: “Avoid used-before-set errors and their associated undefined behavior. Avoid problems with comprehension of complex initialization. Simplify refactoring.”
    • ES.23: Prefer the {}-initializer syntax: “Prefer {}. The rules for {} initialization are simpler, more general, less ambiguous, and safer than for other forms of initialization. Use = only when you are sure that there can be no narrowing conversions. For built-in arithmetic types, use = only with auto. Avoid () initialization, which allows parsing ambiguities.”
11. MarcoFalke removed the label Consensus on May 20, 2020
12. MarcoFalke removed the label RPC/REST/ZMQ on May 20, 2020
13. MarcoFalke removed the label TX fees and policy on May 20, 2020
14. MarcoFalke removed the label Tests on May 20, 2020
15. MarcoFalke removed the label UTXO Db and Indexes on May 20, 2020
16. MarcoFalke added the label Refactoring on May 20, 2020
17. MarcoFalke force-pushed on May 20, 2020
18. net: Use C++11 member initialization in protocol fa8bbb1368
19. MarcoFalke force-pushed on May 20, 2020
20. MarcoFalke marked this as ready for review on May 20, 2020
21. in src/compat/assumptions.h:53 in fa8bbb1368

    49@@ -50,6 +50,7 @@ static_assert(sizeof(double) == 8, "64-bit double assumed");
    50 //             code.
    51 static_assert(sizeof(short) == 2, "16-bit short assumed");
    52 static_assert(sizeof(int) == 4, "32-bit int assumed");
    53+static_assert(sizeof(unsigned) == 4, "32-bit unsigned assumed");
    

    ---

    ---

    

    Empact commented at 9:50 pm on May 20, 2020:

    Wouldn’t unsigned int be the value to test for equivalence?

    ---

    

    sipa commented at 10:03 pm on May 20, 2020:

    unsigned is a shorthand for unsigned int.

    ---

    

    MarcoFalke commented at 10:57 am on May 21, 2020:

    I think this check is redundant anyway, because prepending a type with unsigned shouldn’t change it’s bitlength. But I didn’t feel like looking that up and adding the check has no downsides.
22. 
    laanwj commented at 3:42 pm on May 21, 2020: member

    ACK fa8bbb1368be0f3fd9cc4446aead3f4c2188a4ab Nice cleanup. Checked that nTime’s (100000000) and nServices (NODE_NONE) initial value stays the same in all cases.
23. laanwj merged this on May 21, 2020
24. laanwj closed this on May 21, 2020

    ---

25. MarcoFalke deleted the branch on May 21, 2020
26. sidhujag referenced this in commit 2864507312 on May 21, 2020
27. deadalnix referenced this in commit fcf518ba31 on Feb 9, 2021
28. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
29. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke sipa DrahtBot practicalswift Empact laanwj

Labels
Refactoring P2P