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

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.

fanquake added the label Refactoring on May 16, 2018

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

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

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 :-)

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.

practicalswift force-pushed on May 24, 2018

practicalswift commented at 8:03 PM on May 24, 2018: contributor

Rebased!

practicalswift force-pushed on Jun 2, 2018

ken2812221 commented at 5:21 AM on June 7, 2018: contributor

utACK 0143466

Empact commented at 11:35 PM on June 10, 2018: member

utACK 0143466

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&?

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*?

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&?

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.

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?

practicalswift force-pushed on Jun 14, 2018

practicalswift commented at 9:05 PM on June 14, 2018: contributor

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

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.

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.

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

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

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?

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?

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

All nits, just calling out handling where inconsistent.

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.

practicalswift force-pushed on Jun 15, 2018

practicalswift commented at 8:53 PM on June 15, 2018: contributor

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

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

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.

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.

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

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

practicalswift force-pushed on Jun 17, 2018

practicalswift commented at 10:27 PM on June 17, 2018: contributor

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

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...

practicalswift force-pushed on Jun 18, 2018

practicalswift commented at 6:27 AM on June 18, 2018: contributor

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

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...

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

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

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 &?

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.

practicalswift force-pushed on Jun 18, 2018

practicalswift commented at 2:56 PM on June 18, 2018: contributor

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

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

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).

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.

practicalswift force-pushed on Jun 18, 2018

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? :-)

DrahtBot added the label Needs rebase on Jun 24, 2018

practicalswift force-pushed on Jun 24, 2018

practicalswift commented at 6:38 PM on June 24, 2018: contributor

Rebased!

DrahtBot removed the label Needs rebase on Jun 24, 2018

practicalswift force-pushed on Jun 29, 2018

practicalswift commented at 6:10 AM on June 29, 2018: contributor

Rebased!

DrahtBot added the label Needs rebase on Jul 12, 2018

practicalswift force-pushed on Jul 12, 2018

practicalswift commented at 2:03 PM on July 12, 2018: contributor

Rebased! :-)

DrahtBot removed the label Needs rebase on Jul 12, 2018

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?

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).

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).

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

Comments regarding auto usage.

practicalswift force-pushed on Jul 12, 2018

practicalswift commented at 4:55 PM on July 12, 2018: contributor

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

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

practicalswift force-pushed on Aug 27, 2018

practicalswift commented at 4:20 PM on August 27, 2018: contributor

Rebased! Please re-review :-)

ken2812221 commented at 4:24 AM on September 4, 2018: contributor

utACK f34c8c4

laanwj merged this on Sep 4, 2018

laanwj closed this on Sep 4, 2018

laanwj referenced this in commit 5c24d3b98c on Sep 4, 2018

jasonbcox referenced this in commit 33322f23b3 on Oct 11, 2019

practicalswift deleted the branch on Apr 10, 2021

PastaPastaPasta referenced this in commit 29be02022f on Jul 19, 2021

PastaPastaPasta referenced this in commit a45bab1c06 on Jul 19, 2021

PastaPastaPasta referenced this in commit 112d0ccf21 on Jul 19, 2021

PastaPastaPasta referenced this in commit ff5a94748d on Jul 19, 2021

gades referenced this in commit d8913364f7 on May 9, 2022

DrahtBot locked this on Aug 18, 2022