remove the PAIRTYPE macro

benma commented at 12:15 pm on June 1, 2017: none

Replace all BOOST_FOREACH and Q_FOREACH loops using it with range based loops.

fanquake added the label Refactoring on Jun 1, 2017

practicalswift commented at 12:38 pm on June 1, 2017: contributor

Concept ACK

dcousens approved

fanquake added this to the "In progress" column in a project

fanquake commented at 1:41 pm on June 1, 2017: member

This overlaps with #10193. However given it’s a smaller change-set, and not solely about nuking Boost, maybe we can merge it first? Depends on how many other nearly/ready-to-merge PRs it’s going to break.

benma commented at 1:55 pm on June 1, 2017: none

@fanquake thanks for pointing it out, I wasn’t aware of the other PR. This PR removes the macro and some Q_FOREACH loops over the other one, so it’s a small difference. @jtimon can decide, I guess constantly rebasing such a big PR is a pain, so I don’t mind for his to go first.

in src/utilstrencodings.h:23 in 2ffa099308 outdated

18@@ -19,9 +19,6 @@
19 #define UEND(a)             ((unsigned char*)&((&(a))[1]))
20 #define ARRAYLEN(array)     (sizeof(array)/sizeof((array)[0]))
21 
22-/** This is needed because the foreach macro can't get over the comma in pair<t1, t2> */
23-#define PAIRTYPE(t1, t2)    std::pair<t1, t2>

ryanofsky commented at 3:35 pm on June 1, 2017:

Bizarre that pairtype was defined in utilstrencodings.h.

in src/wallet/wallet.cpp:807 in 2ffa099308 outdated

803@@ -804,7 +804,7 @@ void CWallet::MarkDirty()
804 {
805     {
806         LOCK(cs_wallet);
807-        BOOST_FOREACH(PAIRTYPE(const uint256, CWalletTx)& item, mapWallet)
808+        for(auto& item : mapWallet)

ryanofsky commented at 3:36 pm on June 1, 2017:

Space missing after for

in src/wallet/wallet.cpp:1546 in 2ffa099308 outdated

1542@@ -1543,7 +1543,7 @@ void CWallet::ReacceptWalletTransactions()
1543     }
1544 
1545     // Try to add wallet transactions to memory pool
1546-    BOOST_FOREACH(PAIRTYPE(const int64_t, CWalletTx*)& item, mapSorted)
1547+    for (auto& item : mapSorted)

ryanofsky commented at 3:38 pm on June 1, 2017:

I think this can be const auto.

in src/wallet/wallet.cpp:1811 in 2ffa099308 outdated

1808         if (wtx.nTimeReceived > nTime)
1809             continue;
1810         mapSorted.insert(std::make_pair(wtx.nTimeReceived, &wtx));
1811     }
1812-    BOOST_FOREACH(PAIRTYPE(const unsigned int, CWalletTx*)& item, mapSorted)
1813+    for (auto& item : mapSorted)

ryanofsky commented at 3:39 pm on June 1, 2017:

I think this can be const.

in src/wallet/wallet.cpp:3264 in 2ffa099308 outdated

3260@@ -3261,7 +3261,7 @@ std::map<CTxDestination, CAmount> CWallet::GetAddressBalances()
3261 
3262     {
3263         LOCK(cs_wallet);
3264-        BOOST_FOREACH(PAIRTYPE(uint256, CWalletTx) walletEntry, mapWallet)
3265+        for (auto walletEntry : mapWallet)

ryanofsky commented at 3:45 pm on June 1, 2017:

This is preserving behavior, but copying CWalletTx isn’t ideal. I made a change in #10500 to prevent this.

ryanofsky commented at 4:06 pm on June 1, 2017: member

utACK 2ffa099308e2f3566531e30e5754f83800adbd7d. Nice change. I think it would be good to merge this before #10193 because this change is more targeted and will only make that PR easier to review. I would expect that rebasing #10193 should not be very difficult because most of the commits are scripted.

benma force-pushed on Jun 1, 2017

benma commented at 4:40 pm on June 1, 2017: none

@ryanofsky thanks, I addressed all of your feedback.

ryanofsky commented at 4:44 pm on June 1, 2017: member

utACK 6b50bb066a55d073f870afb12ab57482da725735

jtimon commented at 5:30 pm on June 1, 2017: contributor

Rebasing #10193 isn’t hard at all, once I wrote the sed scripts once it’s just running them again. The problem with #10193 is that the reverse iterator to replace BOOST_REVERSE_FOREACH isn’t working for some reason. So I separated #10502 with the first 3 commits that could be merged right away. I would really prefer to do this using scripted-diff to make sure the removal is complete, on the other hand, it isn’t adding const in the cases where it can like this one.

MarcoFalke commented at 4:21 pm on June 5, 2017: member

Needs rebase

remove the PAIRTYPE macro

Replace all BOOST_FOREACH and Q_FOREACH loops using it with range
based loops.

b3489576c2

benma force-pushed on Jun 10, 2017

benma commented at 2:17 pm on June 10, 2017: none

@MarcoFalke rebased.

practicalswift commented at 2:22 pm on June 10, 2017: contributor

utACK b3489576c28e8e38f1321c1090f7342b75f485c0

Very nice!

sipa commented at 10:53 pm on June 12, 2017: member

Can you write this as a scripted-diff?

jtimon commented at 11:16 pm on June 12, 2017: contributor

Can you write this as a scripted-diff?

Can’t we just merge #10502 instead? Did anybody read #10497 (comment) ?

sipa commented at 1:42 am on June 13, 2017: member

@jtimon Cool!

benma commented at 9:31 am on June 13, 2017: none

As I said before, I don’t mind at all to merge #10502 first. Then I don’t have to repeat the effort of scripting the diff and after rebase this will simply be removing the macro.

laanwj commented at 6:34 pm on June 13, 2017: member

Can’t we just merge #10502 instead?

So: close this in favor of #10502, as it encompasses more and is scripted?

MarcoFalke commented at 6:37 pm on June 13, 2017: member

The only difference is that this one puts auto in place of std::pair<type1, type2>.

On Tue, Jun 13, 2017 at 8:35 PM, Wladimir J. van der Laan < notifications@github.com> wrote:

Can’t we just merge #10502 https://github.com/bitcoin/bitcoin/pull/10502 instead?

So: close this in favor of #10502 https://github.com/bitcoin/bitcoin/pull/10502, as it encompasses more and is scripted?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10497#issuecomment-308208188, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv-c-ROfD9_DFX9h4m3ojKwxHtCiUks5sDtZegaJpZM4Ns5Uv .

benma commented at 10:10 pm on June 13, 2017: none

I think when this PR was started, the other one didn’t deal with the Q_FOREACH and removing the pairtype macro (or I was blind ;)). Now that it does, I’ll close this PR.

(I don’t mind the explicit std::pair annotation).

benma closed this on Jun 13, 2017

fanquake moved this from the "In progress" to the "Done" column in a project

jtimon commented at 2:57 am on June 14, 2017: contributor

#10193 didn’t remove PAIRTYPE until @tjps suggested it in #10193 (review)

DrahtBot locked this on Sep 8, 2021

remove the PAIRTYPE macro #10497