scripted-diff: Remove #include <boost/foreach.hpp> by jtimon · Pull Request #10193 · bitcoin/bitcoin

jtimon · 2017-04-12T04:03:32Z

EDIT: Originally this was only going to remove 'BOOST_FOREACH' and '#include <boost/foreach.hpp>' from src/qr, then src/wallet too, but by popular demand, the scope is increased to fully remove #include <boost/foreach.hpp> the whole project.

Apart from a few small self documented commits in preparation for the scripted commits, the content of the scripted scripts themselves is:

sed -i 's/BOOST_FOREACH *(\(.*\),/for (\1 :/' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ./src/*/*/*.h ./src/*/*/*.cpp ;
sed -i 's/BOOST_REVERSE_FOREACH(\(.*\), \(.*\))/for (\1 : reverse_iterate(\2))/' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ;
sed -i ':a;N;$!ba;s/#include <boost\/foreach.hpp>\n//' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ./src/*/*/*.h ./src/*/*/*.cpp

Dependencies:

devtools/net: add a verifier for scriptable changes. Use it to make CNode::id private. devtools/net: add a verifier for scriptable changes. Use it to make CNode::id private. #10189
scripted-diff: Remove BOOST_FOREACH, Q_FOREACH and PAIRTYPE scripted-diff: Remove BOOST_FOREACH, Q_FOREACH and PAIRTYPE #10502

dcousens · 2017-04-12T04:32:27Z

concept ACK, but maybe follow [after verification/approval] with a removal of the includes too?

practicalswift · 2017-04-12T07:24:35Z

concept ACK

Very good!

jtimon · 2017-04-13T00:15:01Z

Updated OP and code increasing the scope to fully remove #include <boost/foreach.hpp> as discussed here and on IRC.

Although I expect travis to pass, I encountered a problem and "fixed it" by commenting part of prevector_tests.cpp in cc40182 I tried eb83e88 and some trivial edits to the commented lines and I don't know what else to try. If anybody can help with this it would be very welcomed.

If we cannot solve this soon or reverse_iterator.hpp is not acceptable even temporarily for some reason I suggest to go back to remove BOOST_FOREACH everywhere and #include <boost/foreach.hpp> everywhere except for the few places that use BOOST_REVERSE_FOREACH as a first step. But I hope we don't need to do that.

dcousens · 2017-04-13T00:41:05Z

src/reverse_iterator.hpp

is the decltype even needed?

Yes, if I remove it I get ‘begin’ function uses ‘auto’ type specifier without trailing return type.
If I replace the auto with T::reverse_iterator I get need ‘typename’ before ‘T::reverse_iterator’ because ‘T’ is a dependent scope.

@jtimon right, omitting decltype is a C++14 feature - my bad

tjps · 2017-04-13T07:33:13Z

src/init.cpp

PAIRTYPE only exists in service of BOOST_FOREACH, would it make sense to remove it in this PR as well?

gmaxwell · 2017-04-13T19:32:25Z

Luke points out that if only travis is running this it is a vulnerability because we'll be falsely confident in the changes. Other people should run it, but then we're exposed because I assume many people have a workflow that will git pull / git diff and run scripts without reading all the commit messages. spudowiar suggested on IRC that the review script ask you to confirm the script it's going to run. And I think when it's run outside of travis thats what it should do, and it answers the above concerns... reviewers can check this without creating a gratuitous invisible script vector.

maflcko · 2017-04-13T19:58:46Z

The issue that you can not trust travis for the result of the run (i.e. red vs green) is true regardless of the scripted-diff script. Though, I agree with @gmaxwell that travis serves as a good tool to notify pr authors of issues with the commit. Also agree that the interactive approach should prevent most evil scripts from being run.

jtimon · 2017-04-17T19:29:41Z

It seems some more general comments really belong in #10189 .
I agree with the concerns. I think anyone giving an utACK to an "scripted-diff" (or whatever we chose as the prefix) commit should read the script and either review the changes (that's what I did for #10189) or run the script locally. Even if the script in #10189 facilitates review and will help travis warn you when your commit is not complete anymore after rebase (for example, because of new uses of something you are replacing), it is not a replacement for review. I think #10189 's script helps more writers than reviewers (I often want to do "this and nothing else" but some times more things end up in the same commit by accident), but it also establishes a clear format for commit messages for commits using scripts and that makes review easier (because reviewers can run the script themselves or at least review thinking the commit shouldn't do anything else than what the script says [although that king of review doesn't guarantee the change is complete]).

@tjps I tried to remove PAIRTYPE but it seems Q_FOREACH needs it too.
I'm removing Q_FOREACH too to see what people think it seems we may lose some performance with that, see https://www.dvratil.cz/2015/06/qt-containers-and-c11-range-based-loops/
Also, needed rebase.

EDIT: @ipoptika reminder that "UNDO: I really don't understand why this is failing" should get out of the PR (solved somehow) before merging this

jtimon · 2017-06-01T17:26:11Z

Rebased. I still don't know what's wrong with the reverse iterator, so I separated the first 3 commits in #10502

jtimon · 2017-06-02T01:35:02Z

Needed rebase, see #10502 (comment)

-BEGIN VERIFY SCRIPT- sed -i ':a;N;$!ba;s/#include <boost\/foreach.hpp>\n//' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ./src/*/*/*.h ./src/*/*/*.cpp -END VERIFY SCRIPT- Signed-off-by: Pasta <[email protected]>

-BEGIN VERIFY SCRIPT- sed -i 's/BOOST_REVERSE_FOREACH($.*$, $.*$)/for (\1 : reverse_iterate(\2))/' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ; -END VERIFY SCRIPT- Signed-off-by: Pasta <[email protected]>

...where it will be needed Taken from https://gist.github.com/arvidsson/7231973 with small modifications to fit the bitcoin core project

4d4fb33 Rename member field according to the style guide. (Pavel Janík) Pull request description: After bitcoin#10193, approx. five instances of this warning are printed when compiling with `-Wshadow`: ``` In file included from txmempool.cpp:14: ./reverse_iterator.h:20:22: warning: declaration shadows a field of 'reverse_range<T>' [-Wshadow] reverse_range(T &x) : x(x) {} ^ ./reverse_iterator.h:17:8: note: previous declaration is here T &x; ^ 1 warning generated. ``` Tree-SHA512: 6c07c2ed6f4f232a3a8bdcdd6057040967c74552fd29d80f42e8a453b95baf203c410aa31dccc08ff2e765cbba02b1a282f6ea7804955f09b31ab20ef383792e

Backport Boost removal PRs Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7613 - bitcoin/bitcoin#10502 - bitcoin/bitcoin#10193 - bitcoin/bitcoin#13961 - bitcoin/bitcoin#13734 - Only the second commit (we don't need the first). - bitcoin/bitcoin#14480 Part of #4819.

jtimon mentioned this pull request Apr 12, 2017

devtools/net: add a verifier for scriptable changes. Use it to make CNode::id private. #10189

Merged

jtimon force-pushed the b14-10189-scripted-qt-foreach branch from 549107d to f55b6b8 Compare April 12, 2017 04:13

jtimon force-pushed the b14-10189-scripted-qt-foreach branch 2 times, most recently from 4733a94 to 3a40f0b Compare April 12, 2017 05:49

fanquake added Refactoring Scripts and tools labels Apr 12, 2017

jtimon force-pushed the b14-10189-scripted-qt-foreach branch from 3a40f0b to c90307c Compare April 12, 2017 20:04

jtimon changed the title ~~scripted-diff: sed -i 's/BOOST_FOREACH($.*$,/for (\1 :/' ./src/qt/*.cpp~~ scripted-diff: Remove BOOST_FOREACH Apr 12, 2017

jtimon force-pushed the b14-10189-scripted-qt-foreach branch from c90307c to c1b4089 Compare April 12, 2017 23:36

jtimon changed the title ~~scripted-diff: Remove BOOST_FOREACH~~ scripted-diff: #include <boost/foreach.hpp> Apr 13, 2017

jtimon changed the title ~~scripted-diff: #include <boost/foreach.hpp>~~ scripted-diff: Remove #include <boost/foreach.hpp> Apr 13, 2017

jtimon mentioned this pull request Apr 13, 2017

Include cleanup #10181

Merged

dcousens reviewed Apr 13, 2017

View reviewed changes

tjps reviewed Apr 13, 2017

View reviewed changes

jtimon force-pushed the b14-10189-scripted-qt-foreach branch from c1b4089 to f396834 Compare April 17, 2017 19:21

jtimon force-pushed the b14-10189-scripted-qt-foreach branch from f396834 to d0cbbbe Compare April 17, 2017 19:32

jtimon mentioned this pull request May 29, 2017

Util: Remove redundant calls to argsGlobal.IsArgSet() #10118

Merged

1 task

fanquake mentioned this pull request Jun 1, 2017

remove the PAIRTYPE macro #10497

Closed

jtimon force-pushed the b14-10189-scripted-qt-foreach branch from d0cbbbe to aa6e83a Compare June 1, 2017 17:21

jtimon mentioned this pull request Jun 1, 2017

scripted-diff: Remove BOOST_FOREACH, Q_FOREACH and PAIRTYPE #10502

Merged

jtimon force-pushed the b14-10189-scripted-qt-foreach branch 2 times, most recently from 76dd40b to 02298d0 Compare June 2, 2017 01:34

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 24, 2019

\bitcoin#10193 clang-format: Delete ForEachMacros

0fe9f75

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 24, 2019

\bitcoin#10193 Fix const_reverse_iterator constructor (pass const ptr)

96897e5

str4d mentioned this pull request Nov 23, 2020

Backport Boost removal PRs zcash/zcash#4877

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripted-diff: Remove #include <boost/foreach.hpp>#10193

scripted-diff: Remove #include <boost/foreach.hpp>#10193
laanwj merged 5 commits intobitcoin:masterfrom
jtimon:b14-10189-scripted-qt-foreach

jtimon commented Apr 12, 2017 •

edited by fanquake

Loading

Uh oh!

dcousens commented Apr 12, 2017

Uh oh!

practicalswift commented Apr 12, 2017

Uh oh!

jtimon commented Apr 13, 2017

Uh oh!

dcousens Apr 13, 2017

Uh oh!

jtimon Apr 17, 2017

Uh oh!

dcousens Apr 17, 2017 •

edited

Loading

Uh oh!

tjps Apr 13, 2017

Uh oh!

gmaxwell commented Apr 13, 2017

Uh oh!

maflcko commented Apr 13, 2017

Uh oh!

jtimon commented Apr 17, 2017 •

edited

Loading

Uh oh!

jtimon commented Jun 1, 2017

Uh oh!

jtimon commented Jun 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Conversation

jtimon commented Apr 12, 2017 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcousens commented Apr 12, 2017

Uh oh!

practicalswift commented Apr 12, 2017

Uh oh!

jtimon commented Apr 13, 2017

Uh oh!

dcousens Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon Apr 17, 2017

Choose a reason for hiding this comment

Uh oh!

dcousens Apr 17, 2017 • 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!

tjps Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Apr 13, 2017

Uh oh!

maflcko commented Apr 13, 2017

Uh oh!

jtimon commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Jun 1, 2017

Uh oh!

jtimon commented Jun 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

jtimon commented Apr 12, 2017 •

edited by fanquake

Loading

dcousens Apr 17, 2017 •

edited

Loading

jtimon commented Apr 17, 2017 •

edited

Loading