Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. #13249

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:for-const changing 36 files +77 −77
  1. practicalswift commented at 10:01 PM on May 16, 2018: contributor

    Make objects in range declarations immutable by default.

    Rationale:

    • Immutable objects are easier to reason about.
    • Prevents accidental or hard-to-notice change of value.
  2. fanquake added the label Refactoring on May 16, 2018
  3. Empact commented at 7:17 AM on May 17, 2018: member

    How about also including the range-for changes from #12158?

  4. practicalswift commented at 7:37 AM on May 17, 2018: contributor

    @Empact I think it better to do (a subset of) the #12158 changes in follow-up PRs to keep this PR as mechanical and easy-to-review as possible. Basically what @laanwj suggested in #12158 (comment) :-)

    This PR should hopefully be trivial to review :-)

  5. Empact commented at 7:51 AM on May 17, 2018: member

    Yeah, I like it as-is, just suggesting since the & additions wouldn't add much if anything to the line count, so it could be viewed as a form of compression.

    Anyway, you have my Tested ACK 3299ed7. make, make check and test/test_bitcoin ran green locally.

  6. practicalswift force-pushed on May 24, 2018
  7. practicalswift commented at 8:03 PM on May 24, 2018: contributor

    Rebased!

  8. practicalswift force-pushed on Jun 2, 2018
  9. ken2812221 commented at 5:21 AM on June 7, 2018: contributor

    utACK 0143466

  10. DrahtBot cross-referenced this on Jun 7, 2018 from issue [refactor, move-only-ish] Refactor mempool accept/reject logic by skeees
  11. DrahtBot cross-referenced this on Jun 7, 2018 from issue [net,mempool] Call AcceptToMemoryPool() asynchronously in p2p by skeees
  12. Empact commented at 11:35 PM on June 10, 2018: member

    utACK 0143466

  13. in src/miner.cpp:248 in 0143466c93 outdated 
     244 | @@ -245,7 +245,7 @@ int BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& already
 245 |          CTxMemPool::setEntries descendants;
 246 |          mempool.CalculateDescendants(it, descendants);
 247 |          // Insert all descendants (not yet in block) into the modified set
 248 | -        for (CTxMemPool::txiter desc : descendants) {
 249 | +        for (const CTxMemPool::txiter desc : descendants) {
    promag commented at 11:40 PM on June 10, 2018:

    const CTxMemPool::txiter&?

  14. in src/net.cpp:1606 in 0143466c93 outdated 
    1602 | @@ -1603,7 +1603,7 @@ void CConnman::ThreadDNSAddressSeed()
1603 |  
1604 |          LOCK(cs_vNodes);
1605 |          int nRelevant = 0;
1606 | -        for (auto pnode : vNodes) {
1607 | +        for (const auto pnode : vNodes) {
    promag commented at 11:40 PM on June 10, 2018:

    nit, s/auto/CNode*?

  15. in src/net_processing.cpp:853 in 0143466c93 outdated 
     849 | @@ -850,7 +850,7 @@ void PeerLogicValidation::BlockConnected(const std::shared_ptr<const CBlock>& pb
 850 |      // Erase orphan transactions included or precluded by this block
 851 |      if (vOrphanErase.size()) {
 852 |          int nErased = 0;
 853 | -        for (uint256 &orphanHash : vOrphanErase) {
 854 | +        for (const uint256 &orphanHash : vOrphanErase) {
    promag commented at 11:41 PM on June 10, 2018:

    const uint256&?

  16. in src/net_processing.cpp:2275 in 0143466c93 outdated 
    2271 | @@ -2272,7 +2272,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2272 |                  }
2273 |              }
2274 |  
2275 | -            for (uint256 hash : vEraseQueue)
2276 | +            for (const uint256 hash : vEraseQueue)
    promag commented at 11:41 PM on June 10, 2018:

    Same as above.

  17. promag commented at 11:43 PM on June 10, 2018: member

    I've only made some comments where I think the iterated type can be const &. Can you review the whole change if you agree with such change?

  18. DrahtBot cross-referenced this on Jun 14, 2018 from issue gui: Drop qt4 support by laanwj
  19. DrahtBot cross-referenced this on Jun 14, 2018 from issue wallet: Erase wtxOrderd wtx pointer on removeprunedfunds by MarcoFalke
  20. practicalswift force-pushed on Jun 14, 2018
  21. practicalswift force-pushed on Jun 14, 2018
  22. practicalswift force-pushed on Jun 14, 2018
  23. practicalswift commented at 9:05 PM on June 14, 2018: contributor

    @promag Thanks for reviewing! Feedback addressed. Please re-review :-)

  24. in src/policy/rbf.cpp:41 in 9a4655fd15 outdated 
      37 | @@ -38,7 +38,7 @@ RBFTransactionState IsRBFOptIn(const CTransaction &tx, CTxMemPool &pool)
  38 |      CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
  39 |      pool.CalculateMemPoolAncestors(entry, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
  40 |  
  41 | -    for (CTxMemPool::txiter it : setAncestors) {
  42 | +    for (const CTxMemPool::txiter it : setAncestors) {
    Empact commented at 9:19 PM on June 14, 2018:

    nit: Could be a reference too for consistency with src/miner.cpp.

  25. in src/qt/bitcoin.cpp:445 in 9a4655fd15 outdated 
     441 | @@ -442,7 +442,7 @@ void BitcoinApplication::requestShutdown()
 442 |  
 443 |  #ifdef ENABLE_WALLET
 444 |      window->removeAllWallets();
 445 | -    for (WalletModel *walletModel : m_wallet_models) {
 446 | +    for (const WalletModel *walletModel : m_wallet_models) {
    Empact commented at 9:19 PM on June 14, 2018:

    nit: Whitespace

    practicalswift commented at 10:38 PM on June 14, 2018:

    I'm not sure I understand what whitespace change you're suggesting? :-)

    Empact commented at 12:46 AM on June 15, 2018:

    Just that the developer-notes prefer WalletModel*. Fine to ignore.

  26. in src/rpc/blockchain.cpp:535 in 9a4655fd15 outdated 
     531 | @@ -532,14 +532,14 @@ static UniValue getmempoolancestors(const JSONRPCRequest& request)
 532 |  
 533 |      if (!fVerbose) {
 534 |          UniValue o(UniValue::VARR);
 535 | -        for (CTxMemPool::txiter ancestorIt : setAncestors) {
 536 | +        for (const CTxMemPool::txiter ancestorIt : setAncestors) {
    Empact commented at 9:20 PM on June 14, 2018:

    nit: Same for this file

  27. in src/txmempool.cpp:91 in 9a4655fd15 outdated 
      87 | @@ -88,7 +88,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendan
  88 |      int64_t modifySize = 0;
  89 |      CAmount modifyFee = 0;
  90 |      int64_t modifyCount = 0;
  91 | -    for (txiter cit : setAllDescendants) {
  92 | +    for (const txiter cit : setAllDescendants) {
    Empact commented at 9:21 PM on June 14, 2018:

    nit: Same

  28. in src/wallet/wallet.cpp:3213 in 9a4655fd15 outdated 
    3209 | @@ -3210,7 +3210,7 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
3210 |  {
3211 |      AssertLockHeld(cs_wallet); // mapWallet
3212 |      DBErrors nZapSelectTxRet = WalletBatch(*database,"cr+").ZapSelectTx(vHashIn, vHashOut);
3213 | -    for (uint256 hash : vHashOut)
3214 | +    for (const uint256 hash : vHashOut)
    Empact commented at 9:21 PM on June 14, 2018:

    nit: Reference?

  29. in src/wallet/walletdb.cpp:613 in 9a4655fd15 outdated 
     609 | @@ -610,7 +610,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
 610 |      if ((wss.nKeys + wss.nCKeys + wss.nWatchKeys) != wss.nKeyMeta)
 611 |          pwallet->UpdateTimeFirstKey(1);
 612 |  
 613 | -    for (uint256 hash : wss.vWalletUpgrade)
 614 | +    for (const uint256 hash : wss.vWalletUpgrade)
    Empact commented at 9:22 PM on June 14, 2018:

    nit: References?

  30. Empact commented at 9:23 PM on June 14, 2018: member

    All nits, just calling out handling where inconsistent.

  31. sipa commented at 11:16 PM on June 14, 2018: member

    utACK 9a4655fd15e632d95651e4681936f8ea13457ae1

    These changes should be obviously safe. Adding const to variable declarations should never introduce problems (as long as the result compiles). The same is true for converting const variables to references.

  32. DrahtBot cross-referenced this on Jun 15, 2018 from issue Make sure LC_ALL=C is set in all shell scripts by practicalswift
  33. DrahtBot cross-referenced this on Jun 15, 2018 from issue rpc: have verifytxoutproof check the number of txns in proof structure by instagibbs
  34. DrahtBot cross-referenced this on Jun 15, 2018 from issue [WIP] support new multisig template in wallet for Solver, signing, and signature combining by instagibbs
  35. practicalswift force-pushed on Jun 15, 2018
  36. practicalswift force-pushed on Jun 15, 2018
  37. practicalswift commented at 8:53 PM on June 15, 2018: contributor

    @Empact Updated version with nits addressed. @Empact @sipa @promag @ken2812221 Please re-review :-)

  38. Empact commented at 9:14 PM on June 15, 2018: member

    utACK 21ea0e9, though you may get important info by running with the warning from #13480

    Edit: See performance regression noted by @theuni

  39. in src/interfaces/node.cpp:101 in 21ea0e904f outdated 
      97 | @@ -98,7 +98,7 @@ class NodeImpl : public Node
  98 |              g_connman->GetNodeStats(stats_temp);
  99 |  
 100 |              stats.reserve(stats_temp.size());
 101 | -            for (auto& node_stats_temp : stats_temp) {
 102 | +            for (const CNodeStats& node_stats_temp : stats_temp) {
    theuni commented at 9:25 PM on June 15, 2018:

    This would be a performance regression.

    Empact commented at 9:36 PM on June 15, 2018:

    Can you explain? If stats_temp is a std::vector<CNodeStats>, and you can loop over with CNodeStats&, shouldn't you also be able to take a const CNodeStats& without performance implication?

    theuni commented at 9:46 PM on June 15, 2018:

    See below:

    std::move(node_stats_temp)

    This is a non-const reference so that the elements can be moved rather than copied.

    Empact commented at 9:53 PM on June 15, 2018:

    Gotcha, thanks.

  40. theuni commented at 9:37 PM on June 15, 2018: member

    @sipa I disagree. std::move is still allowed on const references, they just... don't move. One such instance of this is here: #13249 (review)

    It's unlikely to cause issues, only performance reductions, but code that relies on refcounts/lifetimes would potentially be affected. So I'm a bit more nervous about such a sweeping change.

  41. sipa commented at 9:46 PM on June 15, 2018: member

    @theuni Of course, I was only commenting on correctness.

  42. practicalswift force-pushed on Jun 17, 2018
  43. practicalswift force-pushed on Jun 17, 2018
  44. practicalswift force-pushed on Jun 17, 2018
  45. practicalswift commented at 10:27 PM on June 17, 2018: contributor

    @theuni Thanks for reviewing. Feedback addressed. Please re-review :-)

  46. in src/policy/rbf.cpp:41 in c622f82c8f outdated 
      37 | @@ -38,7 +38,7 @@ RBFTransactionState IsRBFOptIn(const CTransaction &tx, CTxMemPool &pool)
  38 |      CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
  39 |      pool.CalculateMemPoolAncestors(entry, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
  40 |  
  41 | -    for (CTxMemPool::txiter it : setAncestors) {
  42 | +    for (const CTxMemPool::txiter& it : setAncestors) {
    MarcoFalke commented at 11:46 PM on June 17, 2018:

    Not sure if it makes sense to take a const reference of a iterator...

  47. practicalswift force-pushed on Jun 18, 2018
  48. practicalswift force-pushed on Jun 18, 2018
  49. practicalswift commented at 6:27 AM on June 18, 2018: contributor

    @MarcoFalke Thanks for reviewing. Feedback addressed. Please re-review :-)

  50. MarcoFalke commented at 11:01 AM on June 18, 2018: member

    Again, I am not sure if it makes sense to take a const reference of a iterator...

  51. promag commented at 11:46 AM on June 18, 2018: member

    After some reading I agree with @MarcoFalke, sorry for the suggestion.

  52. practicalswift commented at 2:35 PM on June 18, 2018: contributor

    @MarcoFalke @promag The remaining txiter const ref changes along the line of ...

    -    for (const CTxMemPool::txiter it : …) {
+    for (const CTxMemPool::txiter& it : …) {

    ... are required to make -Wrange-loop-analysis pass.

    Drop const or keep &?

  53. MarcoFalke commented at 2:42 PM on June 18, 2018: member

    Drop const, since CTxMemPool::txiter is already a const iterator and the const does nothing useful, as far as I can see.

  54. practicalswift force-pushed on Jun 18, 2018
  55. practicalswift force-pushed on Jun 18, 2018
  56. practicalswift commented at 2:56 PM on June 18, 2018: contributor

    @MarcoFalke Feedback addressed. Please re-review :-)

  57. practicalswift renamed this:
    Make objects in range declarations immutable by default
    Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations.
    on Jun 18, 2018
  58. sipa commented at 3:36 PM on June 18, 2018: member

    @MarcoFalke I expect it to not matter at all, as in most cases the compiler will optimize things far enough to result in nearly identical code for reference or not.

    I believe the clang warning code suggests references in case a copy can be avoided. I think we should follow those suggestions, as it has the best performance guarantees ignoring optimizations.

    In practice, most iterators are just a single pointer, but some are not (std::vector<bool>'s iterators, for example).

  59. MarcoFalke commented at 3:51 PM on June 18, 2018: member

    @sipa I have no strong opinion on takeing a reference to an iterator, but I believe that using a const iterator instead of a const_iterator is just misleading and could lead to dangerous bugs.

  60. practicalswift force-pushed on Jun 18, 2018
  61. practicalswift commented at 4:02 PM on June 18, 2018: contributor

    Rebased! @MarcoFalke Just to clarify - are you OK with the changes after the latest round of updates? Any outstanding issues? :-)

  62. DrahtBot added the label Needs rebase on Jun 24, 2018
  63. practicalswift force-pushed on Jun 24, 2018
  64. practicalswift commented at 6:38 PM on June 24, 2018: contributor

    Rebased!

  65. DrahtBot removed the label Needs rebase on Jun 24, 2018
  66. practicalswift force-pushed on Jun 29, 2018
  67. practicalswift commented at 6:10 AM on June 29, 2018: contributor

    Rebased!

  68. DrahtBot cross-referenced this on Jul 6, 2018 from issue refactor: add benchmarks to bech32::Encode/Decode by kallewoof
  69. DrahtBot cross-referenced this on Jul 11, 2018 from issue refactor: Optimize bech32::Encode by Empact
  70. DrahtBot added the label Needs rebase on Jul 12, 2018
  71. practicalswift force-pushed on Jul 12, 2018
  72. practicalswift commented at 2:03 PM on July 12, 2018: contributor

    Rebased! :-)

  73. DrahtBot removed the label Needs rebase on Jul 12, 2018
  74. in src/wallet/wallet.cpp:2393 in 6a980ed029 outdated 
    2389 | @@ -2390,7 +2390,7 @@ std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const
2390 |      LOCK2(cs_main, cs_wallet);
2391 |      AvailableCoins(availableCoins);
2392 |  
2393 | -    for (auto& coin : availableCoins) {
2394 | +    for (const auto& coin : availableCoins) {
    promag commented at 2:42 PM on July 12, 2018:

    Could use const COutput& coin?

  75. in src/validation.cpp:4496 in 6a980ed029 outdated 
    4492 | @@ -4493,7 +4493,7 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
4493 |  
4494 |      // Build forward-pointing map of the entire block tree.
4495 |      std::multimap<CBlockIndex*,CBlockIndex*> forward;
4496 | -    for (auto& entry : mapBlockIndex) {
4497 | +    for (const auto& entry : mapBlockIndex) {
    promag commented at 2:44 PM on July 12, 2018:

    Most common usage when iterating mapBlockIndex is

    for (const std::pair<const uint256, CBlockIndex*>& item : mapBlockIndex)

    And there is also the above:

    for (const BlockMap::value_type& entry : mapBlockIndex)

    (I prefer the 1st).

  76. in src/wallet/wallet.cpp:1573 in 6a980ed029 outdated 
    1569 | @@ -1570,7 +1570,7 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
1570 |      // Look up the inputs.  We should have already checked that this transaction
1571 |      // IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our
1572 |      // wallet, with a valid index into the vout array, and the ability to sign.
1573 | -    for (auto& input : tx.vin) {
1574 | +    for (const auto& input : tx.vin) {
    promag commented at 2:48 PM on July 12, 2018:

    Could use const CTxIn& input (most common usage).

  77. promag commented at 2:53 PM on July 12, 2018: member

    Comments regarding auto usage.

  78. practicalswift force-pushed on Jul 12, 2018
  79. practicalswift force-pushed on Jul 12, 2018
  80. practicalswift commented at 4:55 PM on July 12, 2018: contributor

    @promag Thanks for reviewing. Feedback addressed. Please re-review :-)

  81. DrahtBot cross-referenced this on Jul 25, 2018 from issue [WIP] Full unicode support on Windows by ken2812221
  82. DrahtBot cross-referenced this on Jul 26, 2018 from issue [bugfix] Fix encoding issue for Windows by ken2812221
  83. DrahtBot cross-referenced this on Jul 28, 2018 from issue Test for Windows encoding issue by ken2812221
  84. DrahtBot cross-referenced this on Jul 30, 2018 from issue WIP: Transaction Pool Layer by MarcoFalke
  85. DrahtBot cross-referenced this on Jul 31, 2018 from issue [wallet] Kill accounts by jnewbery
  86. DrahtBot cross-referenced this on Aug 22, 2018 from issue Remove accounts rpcs by jnewbery
  87. Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. f34c8c466a
  88. practicalswift force-pushed on Aug 27, 2018
  89. practicalswift commented at 4:20 PM on August 27, 2018: contributor

    Rebased! Please re-review :-)

  90. practicalswift cross-referenced this on Aug 31, 2018 from issue wallet: Remove unused local variable old_label by practicalswift
  91. DrahtBot cross-referenced this on Aug 31, 2018 from issue Minor style enhancement in documentation by fedsten
  92. ken2812221 commented at 4:24 AM on September 4, 2018: contributor

    utACK f34c8c4

  93. laanwj merged this on Sep 4, 2018
  94. laanwj closed this on Sep 4, 2018
  95. laanwj referenced this in commit 5c24d3b98c on Sep 4, 2018
  96. Bushstar cross-referenced this on Oct 11, 2018 from issue Updates from bitcoin/master by Bushstar
  97. jasonbcox referenced this in commit 33322f23b3 on Oct 11, 2019
  98. practicalswift deleted the branch on Apr 10, 2021
  99. PastaPastaPasta referenced this in commit 29be02022f on Jul 19, 2021
  100. PastaPastaPasta referenced this in commit a45bab1c06 on Jul 19, 2021
  101. PastaPastaPasta referenced this in commit 112d0ccf21 on Jul 19, 2021
  102. PastaPastaPasta referenced this in commit ff5a94748d on Jul 19, 2021
  103. gades referenced this in commit d8913364f7 on May 9, 2022
  104. bitcoin locked this on Aug 18, 2022
Contributors
practicalswiftEmpactken2812221promagsipatheuniMarcoFalke
Labels
Refactoring
Linked (view graph)
#12158 Avoid unnecessary copy of objects.#13480 Avoid copies in range-for loops and add a warning to detect them#14117 wallet: Remove unused local variable old_label
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC