Use range-based for loops (C++11) when looping over map elements

[practicalswift]

Before this commit:

for (std::map<T1, T2>::iterator x = y.begin(); x != y.end(); ++x) {
    T1 z = (*x).first;
    …
}

After this commit:

for (auto& x : y) {
    T1 z = x.first;
    …
}

[dcousens]

utACK

[benma]

Should be it.first and it.second.

[practicalswift]

@benma Thanks for reviewing! Fixed! :-)

[benma]

utACK cf02bb1a6c3c0077f1cf37f60e13d6cb71fd9187

[practicalswift]

Rebased!

[sipa]

Overall comment: the loop variables should probably not keep the name it, as that may confuse someone into thinking it refers to an iterator, while it is a container element reference.

[practicalswift]

@sipa Good point! Any suggestion on an appropriate generic variable name to use instead? Perhaps pair in the case of maps or o otherwise?

[sipa]

@practicalswift Perhaps entry for maps?

[practicalswift]

@sipa Ah, yes, entry is much better. Fixing it now!

[practicalswift]

@sipa Fixed! :-)

[maflcko]

Since you are touching all those lines anyway, you can change them to const references for free. Something like the following diff should do:

diff --git a/src/addrdb.h b/src/addrdb.h
index c3d509b..e0a49c8 100644
--- a/src/addrdb.h
+++ b/src/addrdb.h
@@ -61,7 +61,7 @@ public:
         banReason = BanReasonUnknown;
     }
 
-    std::string banReasonToString()
+    std::string banReasonToString() const
     {
         switch (banReason) {
         case BanReasonNodeMisbehaving:
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 0edf111..2f7c014 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -390,9 +390,9 @@ int CAddrMan::Check_()
     if (vRandom.size() != nTried + nNew)
         return -7;
 
-    for (auto& entry : mapInfo) {
+    for (const auto& entry : mapInfo) {
         int n = entry.first;
-        CAddrInfo& info = entry.second;
+        const CAddrInfo& info = entry.second;
         if (info.fInTried) {
             if (!info.nLastSuccess)
                 return -1;
diff --git a/src/net.cpp b/src/net.cpp
index e6c5da8..c071fec 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -106,7 +106,7 @@ bool GetLocal(CService& addr, const CNetAddr *paddrPeer)
     int nBestReachability = -1;
     {
         LOCK(cs_mapLocalHost);
-        for (auto& entry : mapLocalHost)
+        for (const auto& entry : mapLocalHost)
         {
             int nScore = entry.second.nScore;
             int nReachability = entry.first.GetReachabilityFrom(paddrPeer);
@@ -472,10 +472,10 @@ bool CConnman::IsBanned(CNetAddr ip)
     bool fResult = false;
     {
         LOCK(cs_setBanned);
-        for (auto& entry : setBanned)
+        for (const auto& entry : setBanned)
         {
-            CSubNet subNet = entry.first;
-            CBanEntry banEntry = entry.second;
+            const CSubNet& subNet = entry.first;
+            const CBanEntry& banEntry = entry.second;
 
             if(subNet.Match(ip) && GetTime() < banEntry.nBanUntil)
                 fResult = true;
diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
index 5b75886..99c2475 100644
--- a/src/qt/transactiontablemodel.cpp
+++ b/src/qt/transactiontablemodel.cpp
@@ -82,7 +82,7 @@ public:
         cachedWallet.clear();
         {
             LOCK2(cs_main, wallet->cs_wallet);
-            for (auto& entry : wallet->mapWallet)
+            for (const auto& entry : wallet->mapWallet)
             {
                 if (TransactionRecord::showTransaction(entry.second))
                     cachedWallet.append(TransactionRecord::decomposeTransaction(wallet, entry.second));
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 85df33a..345befd 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -570,9 +570,9 @@ UniValue listbanned(const JSONRPCRequest& request)
     g_connman->GetBanned(banMap);
 
     UniValue bannedAddresses(UniValue::VARR);
-    for (auto& entry : banMap)
+    for (const auto& entry : banMap)
     {
-        CBanEntry banEntry = entry.second;
+        const CBanEntry& banEntry = entry.second;
         UniValue rec(UniValue::VOBJ);
         rec.push_back(Pair("address", entry.first.ToString()));
         rec.push_back(Pair("banned_until", banEntry.nBanUntil));
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index db39ac6..6ee1f7d 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -88,7 +88,7 @@ public:
         // Manually recompute the dynamic usage of the whole data, and compare it.
         size_t ret = memusage::DynamicUsage(cacheCoins);
         size_t count = 0;
-        for (auto& entry : cacheCoins) {
+        for (const auto& entry : cacheCoins) {
             ret += entry.second.coin.DynamicMemoryUsage();
             ++count;
         }
@@ -182,7 +182,7 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test)
 
         // Once every 1000 iterations and at the end, verify the full cache.
         if (insecure_rand() % 1000 == 1 || i == NUM_SIMULATION_ITERATIONS - 1) {
-            for (auto& entry : result) {
+            for (const auto& entry : result) {
                 bool have = stack.back()->HaveCoin(entry.first);
                 const Coin& coin = stack.back()->AccessCoin(entry.first);
                 BOOST_CHECK(have == !coin.IsSpent());
@@ -413,7 +413,7 @@ BOOST_AUTO_TEST_CASE(updatecoins_simulation_test)
 
         // Once every 1000 iterations and at the end, verify the full cache.
         if (insecure_rand() % 1000 == 1 || i == NUM_SIMULATION_ITERATIONS - 1) {
-            for (auto& entry : result) {
+            for (const auto& entry : result) {
                 bool have = stack.back()->HaveCoin(entry.first);
                 const Coin& coin = stack.back()->AccessCoin(entry.first);
                 BOOST_CHECK(have == !coin.IsSpent());
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index a5143aa..e85732e 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -1263,7 +1263,7 @@ UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool fByA
 
     if (fByAccounts)
     {
-        for (auto& entry : mapAccountTally)
+        for (const auto& entry : mapAccountTally)
         {
             CAmount nAmount = entry.second.nAmount;
             int nConf = entry.second.nConf;

[practicalswift]

@MarcoFalke Good idea! You're suggestions are now part of the PR :-)

[practicalswift]

Merge conflicts resolved!

[sipa]

Needs rebase

[practicalswift]

Rebased!

[jtimon]

utACK 875ebee20e048892d999c41acb7100d0fdac1f91

[sipa]

I believe there may be a few more possible consts missing.

[sipa]

I think this can be const

[sipa]

Can be const (the loop modifies the entries pointed to by mapBlockIndex, not the entries directly.

[sipa]

Same, can be const.

[practicalswift]

@promag Yes, but with an additional condition (account.vchPubKey.IsValid()). The scope of this PR is to cover the simplest possible case only (for (std::map<T1, T2>::iterator x = y.begin(); x != y.end(); ++x) {) to make reviewing trivial.

[ajtowns]

TestedACK 680bc2c. Looks good to me. There's a bunch of untouched for()'s with vector::iterator and set::iterator's that could presumably have similar refactors in a later commit.

[practicalswift]

@ajtowns Thanks for the review and the tested ACK! This PR is intentionally only touching std::map to limit the scope and make it easy to review :-)

[practicalswift]

Does this PR stand a chance of being merged? If not I'll close it :-)

[sipa]

utACK 680bc2c

[maflcko]

utACK 680bc2c

[practicalswift]

Ready for merge? :-)