Use C++11 member initializer in CNodeState #21370

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2103-netCNodeStateRefactor changing 1 files +18 −35

MarcoFalke commented at 10:46 AM on March 5, 2021: member

This removes a bunch of boilerplate, makes the code easier to read. Also, C++11 member initialization avoids accidental uninitialized members.
Use C++11 member initializer in CNodeState fa476f188e
fanquake added the label Refactoring on Mar 5, 2021

in src/net_processing.cpp:496 in fa476f188e

 490 | @@ -491,47 +491,47 @@ struct CNodeState {
 491 |      //! The peer's address
 492 |      const CService address;
 493 |      //! The best known block we know this peer has announced.
 494 | -    const CBlockIndex *pindexBestKnownBlock;
 495 | +    const CBlockIndex* pindexBestKnownBlock{nullptr};
 496 |      //! The hash of the last unknown block this peer has announced.
 497 | -    uint256 hashLastUnknownBlock;
 498 | +    uint256 hashLastUnknownBlock{};

hebasto commented at 11:12 AM on March 5, 2021:

why not

    uint256 hashLastUnknownBlock{0};

hebasto commented at 11:17 AM on March 5, 2021:

or use uint256::ZERO ?

MarcoFalke commented at 3:31 PM on March 5, 2021:

I think all three variants are the same, it shouldn't matter which one is picked

hebasto approved
hebasto commented at 3:55 PM on March 5, 2021: member

cr ACK fa476f188ed1200087ea4cca2e883c792836f845
practicalswift commented at 9:55 PM on March 8, 2021: contributor

Safer, more readable and shorter (+18 −35): nice! :)

cr ACK fa476f188ed1200087ea4cca2e883c792836f845

I'd be happy to review any PR migrating our classes to using C++11 default member initialization: safer is better! :)
in src/net_processing.cpp:571 in fa476f188e
```
 567 | @@ -568,13 +568,13 @@ struct CNodeState {
 568 |          bool m_protect;
 569 |      };
 570 |  
 571 | -    ChainSyncTimeoutState m_chain_sync;
 572 | +    ChainSyncTimeoutState m_chain_sync{0, nullptr, false, false};
```
jnewbery commented at 9:34 AM on March 9, 2021:

Any reason not to add these default initializers to the ChainSyncTimeoutState members themselves?

jnewbery commented at 11:46 AM on March 12, 2021:

Done in #21424
in src/net_processing.cpp:492 in fa476f188e
```
 490 | @@ -491,47 +491,47 @@ struct CNodeState {
 491 |      //! The peer's address
 492 |      const CService address;
```
jnewbery commented at 9:35 AM on March 9, 2021:

This address member is now redundant and can be removed (from here and the initializer list). I'll do that in a follow-up.

jnewbery commented at 11:45 AM on March 12, 2021:

Fixed in #21395

in src/net_processing.cpp:588 in fa476f188e

 600 | -        fHaveWitness = false;
 601 | -        fWantsCmpctWitness = false;
 602 | -        fSupportsDesiredCmpctVersion = false;
 603 | -        m_chain_sync = { 0, nullptr, false, false };
 604 | -        m_last_block_announcement = 0;
 605 |          m_recently_announced_invs.reset();

jnewbery commented at 9:36 AM on March 9, 2021:

Any reason to keep this? A freshly initialized CRollingBloomFilter is reset in its ctor.

jnewbery commented at 11:46 AM on March 12, 2021:

Done in #21424

jnewbery commented at 9:36 AM on March 9, 2021: member

utACK fa476f188ed1200087ea4cca2e883c792836f845
fanquake merged this on Mar 9, 2021
fanquake closed this on Mar 9, 2021
MarcoFalke deleted the branch on Mar 9, 2021
sidhujag referenced this in commit 60d7198cf7 on Mar 9, 2021
MarcoFalke referenced this in commit c771fc0dc1 on Mar 15, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#21424 Net processing: Tidy up CNodeState ctor