remove the PAIRTYPE macro #10497

pull benma wants to merge 1 commits into bitcoin:master from benma:pairtype changing 15 files +30 −33
  1. 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.

  2. fanquake added the label Refactoring on Jun 1, 2017
  3. practicalswift commented at 12:38 PM on June 1, 2017: contributor

    Concept ACK

  4. dcousens approved
  5. fanquake added this to the "In progress" column in a project
  6. 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.

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

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

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

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

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

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

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

  14. benma force-pushed on Jun 1, 2017
  15. benma commented at 4:40 PM on June 1, 2017: none

    @ryanofsky thanks, I addressed all of your feedback.

  16. ryanofsky commented at 4:44 PM on June 1, 2017: member

    utACK 6b50bb066a55d073f870afb12ab57482da725735

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

  18. MarcoFalke commented at 4:21 PM on June 5, 2017: member

    Needs rebase

  19. remove the PAIRTYPE macro
    Replace all BOOST_FOREACH and Q_FOREACH loops using it with range
based loops.
    b3489576c2
  20. benma force-pushed on Jun 10, 2017
  21. benma commented at 2:17 PM on June 10, 2017: none

    @MarcoFalke rebased.

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

    utACK b3489576c28e8e38f1321c1090f7342b75f485c0

    Very nice!

  23. sipa commented at 10:53 PM on June 12, 2017: member

    Can you write this as a scripted-diff?

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

  25. sipa commented at 1:42 AM on June 13, 2017: member

    @jtimon Cool!

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

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

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

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

  30. benma closed this on Jun 13, 2017
  31. fanquake moved this from the "In progress" to the "Done" column in a project
  32. jtimon commented at 2:57 AM on June 14, 2017: contributor

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

  33. DrahtBot locked this on Sep 8, 2021
Contributors
benmapracticalswiftdcousensfanquakeryanofskyjtimonMarcoFalkesipalaanwj
Labels
Refactoring
Linked (view graph)
#10502 scripted-diff: Remove BOOST_FOREACH, Q_FOREACH and PAIRTYPE
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-04-13 21:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me