Use C++11 member initializer in CTxMemPoolEntry #23054

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2109-cpp11MemberInitMempool changing 2 files +28 −31

MarcoFalke commented at 12:13 PM on September 21, 2021: member

This removes a bunch of boilerplate, makes the code easier to read. Also, C++11 member initialization avoids accidental uninitialized members.

Can be reviewed with the git option "--word-diff-regex=." or with "git difftool --tool=meld".
MarcoFalke cross-referenced this on Sep 21, 2021 from issue doc: Fix incorrect C++ named args by MarcoFalke
fanquake added the label Mempool on Sep 21, 2021
fanquake added the label Refactoring on Sep 21, 2021
laanwj commented at 12:19 PM on September 21, 2021: member

Code review ACK fa922062c2eb122e6c4972f20b67723332adcb8c

in src/txmempool.h:118 in fa922062c2 outdated

 114 | @@ -115,10 +115,10 @@ class CTxMemPoolEntry
 115 |      int64_t nSigOpCostWithAncestors;
 116 |  
 117 |  public:
 118 | -    CTxMemPoolEntry(const CTransactionRef& _tx, const CAmount& _nFee,
 119 | -                    int64_t _nTime, unsigned int _entryHeight,
 120 | -                    bool spendsCoinbase,
 121 | -                    int64_t nSigOpsCost, LockPoints lp);
 122 | +    CTxMemPoolEntry(const CTransactionRef& tx, const CAmount& fee,

jonatack commented at 12:38 PM on September 21, 2021:

Feel free to ignore but if you retouch, while changing this function declaration and definition, could also do

    CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,

MarcoFalke commented at 1:59 PM on September 21, 2021:

Thanks, done

jonatack commented at 12:39 PM on September 21, 2021: contributor

Approach ACK
hebasto commented at 12:39 PM on September 21, 2021: member

Concept ACK since #17786 :)

in src/txmempool.cpp:40 in fa922062c2 outdated

  55 | +      nSizeWithDescendants{GetTxSize()},
  56 | +      nModFeesWithDescendants{nFee},
  57 | +      nCountWithAncestors{1},
  58 | +      nSizeWithAncestors{GetTxSize()},
  59 | +      nModFeesWithAncestors{nFee},
  60 | +      nSigOpCostWithAncestors{sigOpCost} {}

jnewbery commented at 1:16 PM on September 21, 2021:

Sorry for painting the bikeshed, but wouldn't standard project style be to put the body of the function on a new line?

      nSigOpCostWithAncestors{sigOpCost}
  {}

MarcoFalke commented at 2:00 PM on September 21, 2021:

Clang-format accepts the trailing {}, so I think it is fine

in src/txmempool.cpp:35 in fa922062c2 outdated
```
  47 | +      nUsageSize{RecursiveDynamicUsage(tx)},
  48 | +      nTime{time},
  49 | +      entryHeight{entry_height},
  50 | +      spendsCoinbase{spends_coinbase},
  51 | +      sigOpCost{sigops_cost},
  52 | +      feeDelta{0},
```
jnewbery commented at 1:17 PM on September 21, 2021:

What do you think about initializing this (and perhaps nCountWithDescendants and nCountWithAncestors) with default initialization rather than in the initializer list?

MarcoFalke commented at 1:59 PM on September 21, 2021:

Thanks, done
jnewbery commented at 1:17 PM on September 21, 2021: member

Concept ACK
MarcoFalke force-pushed on Sep 21, 2021

Use C++11 member initializer in CTxMemPoolEntry

This removes a bunch of boilerplate, makes the code easier to read.
Also, C++11 member initialization avoids accidental uninitialized
members.

Can be reviewed with the git option "--word-diff-regex=." or with "git
difftool --tool=meld".

fa08d4cfb1

MarcoFalke force-pushed on Sep 21, 2021
MarcoFalke commented at 2:05 PM on September 21, 2021: member

Concept ACK since #17786 :)

Thanks, took the other diff from there.
shaavan approved
shaavan commented at 2:18 PM on September 21, 2021: contributor

Code Review ACK fa08d4cfb1e3447a76093d46edc1c9de5ceee454

This PR simplifies the code, removing unnecessary parts, at the same time, keeping the same functionality. The involved constants are now initialized through default initialization rather than from the function. The Clang Formatting of the PR is correct. So there are no formatting issues either. Thanks, @MarcoFalke, for catching these redundancies.
jnewbery commented at 2:42 PM on September 21, 2021: member

Code review ACK fa08d4cfb1e3447a76093d46edc1c9de5ceee454
MarcoFalke commented at 5:10 PM on September 21, 2021: member

Failing CI can be ignored

in src/txmempool.h:109 in fa08d4cfb1

 107 |      uint64_t nSizeWithDescendants;   //!< ... and size
 108 |      CAmount nModFeesWithDescendants; //!< ... and total fees (all including us)
 109 |  
 110 |      // Analogous statistics for ancestor transactions
 111 | -    uint64_t nCountWithAncestors;
 112 | +    uint64_t nCountWithAncestors{1};

JeremyRubin commented at 5:29 PM on September 21, 2021:

I don't see a reason to not use the initializer list for these fields, seems more consistent style wise.

MarcoFalke commented at 6:42 PM on September 21, 2021:

I changed this because several people asked me to, see #23054 (review)

fanquake merged this on Sep 23, 2021
fanquake closed this on Sep 23, 2021
MarcoFalke deleted the branch on Sep 23, 2021
sidhujag referenced this in commit 0cd8578262 on Sep 24, 2021
hebasto commented at 5:15 PM on September 24, 2021: member

Post-merge ACK fa08d4cfb1e3447a76093d46edc1c9de5ceee454.
hebasto cross-referenced this on Sep 24, 2021 from issue refactor: Nuke policy/fees->mempool circular dependencies by hebasto
Fabcien referenced this in commit 9731bc95ab on Oct 7, 2022
bitcoin locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

#17786 refactor: Nuke policy/fees->mempool circular dependencies #22981 doc: Fix incorrect C++ named args