Prefer initialization to assignment in constructors. Prefer in-class initializers to member initializers in constructors for constant initializers. #13766

pull practicalswift wants to merge 5 commits into bitcoin:master from practicalswift:initialize-members-in-initialization-list changing 15 files +87 −164

practicalswift commented at 2:09 PM on July 26, 2018: contributor
Initialize members in initialization lists. Prefer in-class initializers to member initializers in constructors for constant initializers.

Rationale:
- C.49: Prefer initialization to assignment in constructors
- C.48: Prefer in-class initializers to member initializers in constructors for constant initializers
fanquake added the label Refactoring on Jul 26, 2018
DrahtBot commented at 2:54 PM on July 26, 2018: member
Reviewers, this pull request conflicts with the following ones:
- #14046 (net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli)
- #13123 (net: Add Clang thread safety annotations for guarded variables in the networking code by practicalswift)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/primitives/transaction.cpp:21 in b61c902ae6 outdated

  21 | -    scriptSig = scriptSigIn;
  22 | -    nSequence = nSequenceIn;
  23 |  }
  24 |  
  25 | -CTxIn::CTxIn(uint256 hashPrevTx, uint32_t nOut, CScript scriptSigIn, uint32_t nSequenceIn)
  26 | +CTxIn::CTxIn(uint256 hashPrevTx, uint32_t nOut, CScript scriptSigIn, uint32_t nSequenceIn) : prevout(COutPoint(hashPrevTx, nOut)), scriptSig(scriptSigIn), nSequence(nSequenceIn)

MarcoFalke commented at 3:53 PM on July 26, 2018:

Could put a \n before the : to avoid having excessively long lines, no? (Feddback also applies to other places with long lines)

practicalswift force-pushed on Jul 26, 2018
practicalswift commented at 4:21 PM on July 26, 2018: contributor

@MarcoFalke Good point! Newlines added. Please re-review :-)
promag commented at 5:21 PM on July 26, 2018: member

Concept ACK.

in src/wallet/wallet.cpp:4343 in 5e80e9345c outdated

4356 | +CKeyPool::CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn) : vchPubKey(vchPubKeyIn), fInternal(internalIn)
4357 |  {
4358 |      nTime = GetTime();
4359 | -    vchPubKey = vchPubKeyIn;
4360 | -    fInternal = internalIn;
4361 |      m_pre_split = false;

Empact commented at 9:38 PM on July 26, 2018:

nit: this one too?

in src/script/standard.cpp:260 in 5e80e9345c outdated
```
 255 | @@ -256,7 +256,8 @@ class CScriptVisitor : public boost::static_visitor<bool>
 256 |  private:
 257 |      CScript *script;
 258 |  public:
 259 | -    explicit CScriptVisitor(CScript *scriptin) { script = scriptin; }
 260 | +    explicit CScriptVisitor(CScript *scriptin) : script(scriptin) {
 261 | +    }
```
Empact commented at 9:39 PM on July 26, 2018:

nit: whitespace

practicalswift commented at 10:01 PM on July 26, 2018:

@Empact Where do you want the whitespace added/removed? :-)

MarcoFalke commented at 10:04 PM on July 26, 2018:

Probably leave everything in a single line?
Empact commented at 10:09 PM on July 26, 2018:

clang-format-diff.py accepts at least:

explicit CScriptVisitor(CScript* scriptin) : script(scriptin) {} explicit CScriptVisitor(CScript* scriptin) : script(scriptin) { }

For an empty set, like Marco I like the former.
Empact commented at 10:12 PM on July 26, 2018:

Could also apply to CompareInvMempoolOrder, FreespaceChecker, etc.
practicalswift force-pushed on Jul 26, 2018
practicalswift force-pushed on Aug 2, 2018
practicalswift force-pushed on Aug 2, 2018
practicalswift force-pushed on Aug 2, 2018
practicalswift force-pushed on Aug 2, 2018
practicalswift commented at 11:57 AM on August 2, 2018: contributor

@Empact Thanks for reviewing. Feedback addressed. Please re-review :-)
laanwj commented at 12:27 PM on August 29, 2018: member

Some of these initializers (those with constant values) could move to the class declaration itself
practicalswift force-pushed on Aug 29, 2018
practicalswift force-pushed on Aug 29, 2018
practicalswift force-pushed on Aug 30, 2018
practicalswift force-pushed on Aug 30, 2018
practicalswift renamed this:
~~Initialize members in initialization lists~~
Prefer initialization to assignment in constructors. Prefer in-class initializers to member initializers in constructors for constant initializers.
on Aug 30, 2018
practicalswift force-pushed on Aug 30, 2018
practicalswift commented at 9:41 AM on August 30, 2018: contributor

@laanwj Now preferring in-class initializers to member initializers in constructors for constant initializers. Please re-review the PR.

Please note that a redundant call to filterInventoryKnown.reset() has been removed. That looks correct, right?
practicalswift force-pushed on Aug 30, 2018
practicalswift commented at 9:53 AM on August 30, 2018: contributor

Also added a developer note: "Prefer initialization to assignment in constructors"
Prefer initialization to assignment in constructors 6a93312c88
Prefer in-class initializers to member initializers in constructors for constant initializers 3d7db2b35d
Remove redundant filterInventoryKnown.reset() a7d8a96fd3
Add developer note: Prefer initialization to assignment in constructors 9e65bd423e
Pass uint256 hashPrevTx as const reference 157ab566a6
practicalswift force-pushed on Sep 5, 2018
practicalswift closed this on Oct 22, 2018
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

Labels