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

practicalswift · 2018-05-16T22:01:37Z

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.

Empact · 2018-05-17T07:17:36Z

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

practicalswift · 2018-05-17T07:37:42Z

@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 · 2018-05-17T07:51:24Z

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 · 2018-05-24T20:03:38Z

Rebased!

ken2812221 · 2018-06-07T05:21:34Z

utACK 0143466

Empact · 2018-06-10T23:35:21Z

utACK 0143466

promag

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?

promag · 2018-06-10T23:40:19Z

src/miner.cpp

@@ -245,7 +245,7 @@ int BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& already
        CTxMemPool::setEntries descendants;
        mempool.CalculateDescendants(it, descendants);
        // Insert all descendants (not yet in block) into the modified set
-        for (CTxMemPool::txiter desc : descendants) {
+        for (const CTxMemPool::txiter desc : descendants) {


const CTxMemPool::txiter&?

promag · 2018-06-10T23:40:49Z

src/net.cpp

@@ -1603,7 +1603,7 @@ void CConnman::ThreadDNSAddressSeed()

        LOCK(cs_vNodes);
        int nRelevant = 0;
-        for (auto pnode : vNodes) {
+        for (const auto pnode : vNodes) {


nit, s/auto/CNode*?

promag · 2018-06-10T23:41:07Z

src/net_processing.cpp

@@ -850,7 +850,7 @@ void PeerLogicValidation::BlockConnected(const std::shared_ptr<const CBlock>& pb
    // Erase orphan transactions included or precluded by this block
    if (vOrphanErase.size()) {
        int nErased = 0;
-        for (uint256 &orphanHash : vOrphanErase) {
+        for (const uint256 &orphanHash : vOrphanErase) {


const uint256&?

promag · 2018-06-10T23:41:13Z

src/net_processing.cpp

@@ -2272,7 +2272,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                }
            }

-            for (uint256 hash : vEraseQueue)
+            for (const uint256 hash : vEraseQueue)


Same as above.

practicalswift · 2018-06-14T21:05:22Z

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

Empact · 2018-06-14T21:19:33Z

src/policy/rbf.cpp

@@ -38,7 +38,7 @@ RBFTransactionState IsRBFOptIn(const CTransaction &tx, CTxMemPool &pool)
    CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
    pool.CalculateMemPoolAncestors(entry, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false);

-    for (CTxMemPool::txiter it : setAncestors) {
+    for (const CTxMemPool::txiter it : setAncestors) {


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

Empact · 2018-06-14T21:19:39Z

src/qt/bitcoin.cpp

@@ -442,7 +442,7 @@ void BitcoinApplication::requestShutdown()

 #ifdef ENABLE_WALLET
    window->removeAllWallets();
-    for (WalletModel *walletModel : m_wallet_models) {
+    for (const WalletModel *walletModel : m_wallet_models) {


nit: Whitespace

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

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

Empact · 2018-06-14T21:20:26Z

src/rpc/blockchain.cpp

@@ -532,14 +532,14 @@ static UniValue getmempoolancestors(const JSONRPCRequest& request)

    if (!fVerbose) {
        UniValue o(UniValue::VARR);
-        for (CTxMemPool::txiter ancestorIt : setAncestors) {
+        for (const CTxMemPool::txiter ancestorIt : setAncestors) {


nit: Same for this file

Empact · 2018-06-14T21:21:01Z

src/txmempool.cpp

@@ -88,7 +88,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendan
    int64_t modifySize = 0;
    CAmount modifyFee = 0;
    int64_t modifyCount = 0;
-    for (txiter cit : setAllDescendants) {
+    for (const txiter cit : setAllDescendants) {


Empact · 2018-06-14T21:21:42Z

src/wallet/wallet.cpp

@@ -3210,7 +3210,7 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
 {
    AssertLockHeld(cs_wallet); // mapWallet
    DBErrors nZapSelectTxRet = WalletBatch(*database,"cr+").ZapSelectTx(vHashIn, vHashOut);
-    for (uint256 hash : vHashOut)
+    for (const uint256 hash : vHashOut)


nit: Reference?

Empact · 2018-06-14T21:22:10Z

src/wallet/walletdb.cpp

@@ -610,7 +610,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
    if ((wss.nKeys + wss.nCKeys + wss.nWatchKeys) != wss.nKeyMeta)
        pwallet->UpdateTimeFirstKey(1);

-    for (uint256 hash : wss.vWalletUpgrade)
+    for (const uint256 hash : wss.vWalletUpgrade)


nit: References?

Empact · 2018-06-14T21:23:42Z

All nits, just calling out handling where inconsistent.

sipa · 2018-06-14T23:16:59Z

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 · 2018-07-12T14:03:38Z

Rebased! :-)

promag

Comments regarding auto usage.

promag · 2018-07-12T14:42:55Z

src/wallet/wallet.cpp

@@ -2390,7 +2390,7 @@ std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const
    LOCK2(cs_main, cs_wallet);
    AvailableCoins(availableCoins);

-    for (auto& coin : availableCoins) {
+    for (const auto& coin : availableCoins) {


Could use const COutput& coin?

promag · 2018-07-12T14:44:40Z

src/validation.cpp

@@ -4493,7 +4493,7 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)

    // Build forward-pointing map of the entire block tree.
    std::multimap<CBlockIndex*,CBlockIndex*> forward;
-    for (auto& entry : mapBlockIndex) {
+    for (const auto& entry : mapBlockIndex) {


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

promag · 2018-07-12T14:48:44Z

src/wallet/wallet.cpp

@@ -1570,7 +1570,7 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
    // Look up the inputs.  We should have already checked that this transaction
    // IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our
    // wallet, with a valid index into the vout array, and the ability to sign.
-    for (auto& input : tx.vin) {
+    for (const auto& input : tx.vin) {


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

practicalswift · 2018-07-12T16:55:14Z

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

…ssary copying of objects in range declarations.

practicalswift · 2018-08-27T16:20:52Z

Rebased! Please re-review :-)

ken2812221 · 2018-09-04T04:24:46Z

utACK f34c8c4

…. Avoid unnecessary copying of objects in range declarations. f34c8c4 Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. (practicalswift) Pull request description: 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. Tree-SHA512: cad69d35f0cf8a938b848e65dd537c621d96fe3369be306b65ef0cd1baf6cc0a9f28bc230e1e383d810c555a6743d08cb6b2b0bd51856d4611f537a12e5abb8b

…ssary copying of objects in range declarations. Summary: Make objects in range declarations immutable by default. Backport of Bitcoin Core PR13249 bitcoin/bitcoin#13249 (D4191 done again) Test Plan: ``` make check-all ``` Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4221

…default. Avoid unnecessary copying of objects in range declarations. f34c8c4 Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. (practicalswift) Pull request description: 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. Tree-SHA512: cad69d35f0cf8a938b848e65dd537c621d96fe3369be306b65ef0cd1baf6cc0a9f28bc230e1e383d810c555a6743d08cb6b2b0bd51856d4611f537a12e5abb8b

fanquake added the Refactoring label May 16, 2018

practicalswift force-pushed the for-const branch from 3299ed7 to d6bb0bf Compare May 24, 2018 20:03

practicalswift force-pushed the for-const branch from d6bb0bf to 0143466 Compare June 2, 2018 17:37

This was referenced Jun 7, 2018

[refactor, move-only-ish] Refactor mempool accept/reject logic #13407

Closed

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

promag reviewed Jun 10, 2018

View reviewed changes

This was referenced Jun 14, 2018

gui: Drop qt4 support #13458

Merged

wallet: Erase wtxOrderd wtx pointer on removeprunedfunds #13437

Merged

practicalswift force-pushed the for-const branch 3 times, most recently from 3e16523 to 9a4655f Compare June 14, 2018 21:03

Empact reviewed Jun 14, 2018

View reviewed changes

This was referenced Jun 15, 2018

Make sure LC_ALL=C is set in all shell scripts #13454

Merged

rpc: have verifytxoutproof check the number of txns in proof structure #13452

Merged

[WIP] support new multisig template in wallet for Solver, signing, and signature combining #13449

Closed

practicalswift force-pushed the for-const branch from 9a4655f to 2b132fe Compare June 15, 2018 20:10

practicalswift force-pushed the for-const branch from ea2cd74 to 6a980ed Compare July 12, 2018 14:02

DrahtBot removed the Needs rebase label Jul 12, 2018

promag reviewed Jul 12, 2018

View reviewed changes

practicalswift force-pushed the for-const branch 2 times, most recently from 279caa6 to ddd0045 Compare July 12, 2018 16:54

This was referenced Jul 25, 2018

[WIP] Full unicode support on Windows #13755

Closed

[bugfix] Fix encoding issue for Windows #13426

Closed

Test for Windows encoding issue #13787

Merged

WIP: Transaction Pool Layer #13804

Closed

[wallet] Kill accounts #13825

Closed

DrahtBot mentioned this pull request Aug 22, 2018

Remove accounts rpcs #14023

Merged

Make objects in range declarations immutable by default. Avoid unnece…

f34c8c4

…ssary copying of objects in range declarations.

practicalswift force-pushed the for-const branch from ddd0045 to f34c8c4 Compare August 27, 2018 16:20

practicalswift mentioned this pull request Aug 31, 2018

wallet: Remove unused local variable old_label #14117

Closed

DrahtBot mentioned this pull request Aug 31, 2018

Minor style enhancement in documentation #14091

Closed

laanwj merged commit f34c8c4 into bitcoin:master Sep 4, 2018

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

practicalswift deleted the for-const branch April 10, 2021 19:35

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

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

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

Uh oh!

Conversation

practicalswift commented May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented May 17, 2018

Uh oh!

practicalswift commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented May 24, 2018

Uh oh!

ken2812221 commented Jun 7, 2018

Uh oh!

Empact commented Jun 10, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jun 14, 2018

Uh oh!

Empact Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented Jun 14, 2018

Uh oh!

sipa commented Jun 14, 2018

Uh oh!

practicalswift commented Jul 12, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jul 12, 2018

Uh oh!

practicalswift commented Aug 27, 2018

practicalswift commented May 16, 2018 •

edited

Loading

practicalswift commented May 17, 2018 •

edited

Loading

Empact commented May 17, 2018 •

edited

Loading

Empact Jun 14, 2018 •

edited

Loading

Empact Jun 14, 2018 •

edited

Loading

Empact Jun 14, 2018 •

edited

Loading

Empact Jun 14, 2018 •

edited

Loading

Empact Jun 14, 2018 •

edited

Loading

Empact Jun 14, 2018 •

edited

Loading