Skip to content

Conversation

@benma
Copy link

@benma benma commented Jun 1, 2017

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

@practicalswift
Copy link
Contributor

Concept ACK

@fanquake
Copy link
Member

fanquake commented Jun 1, 2017

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
Copy link
Author

benma commented Jun 1, 2017

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

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bizarre that pairtype was defined in utilstrencodings.h.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space missing after for

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be const auto.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be const.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@benma
Copy link
Author

benma commented Jun 1, 2017

@ryanofsky thanks, I addressed all of your feedback.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 6b50bb066a55d073f870afb12ab57482da725735

@jtimon
Copy link
Contributor

jtimon commented Jun 1, 2017

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.

@maflcko
Copy link
Member

maflcko commented Jun 5, 2017

Needs rebase

Replace all BOOST_FOREACH and Q_FOREACH loops using it with range
based loops.
@benma
Copy link
Author

benma commented Jun 10, 2017

@MarcoFalke rebased.

@practicalswift
Copy link
Contributor

utACK b348957

Very nice!

@sipa
Copy link
Member

sipa commented Jun 12, 2017

Can you write this as a scripted-diff?

@jtimon
Copy link
Contributor

jtimon commented Jun 12, 2017

Can you write this as a scripted-diff?

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

@sipa
Copy link
Member

sipa commented Jun 13, 2017

@jtimon Cool!

@benma
Copy link
Author

benma commented Jun 13, 2017

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
Copy link
Member

laanwj commented Jun 13, 2017

Can't we just merge #10502 instead?

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

@maflcko
Copy link
Member

maflcko commented Jun 13, 2017 via email

@benma
Copy link
Author

benma commented Jun 13, 2017

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 benma closed this Jun 13, 2017
@jtimon
Copy link
Contributor

jtimon commented Jun 14, 2017

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

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants