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

pull jtimon wants to merge 5 commits into bitcoin:master from jtimon:b14-10189-scripted-qt-foreach changing 38 files +53 −39
  1. jtimon commented at 4:03 AM on April 12, 2017: contributor

    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. #10189
    • scripted-diff: Remove BOOST_FOREACH, Q_FOREACH and PAIRTYPE #10502
  2. jtimon cross-referenced this on Apr 12, 2017 from issue devtools/net: add a verifier for scriptable changes. Use it to make CNode::id private. by theuni
  3. jtimon force-pushed on Apr 12, 2017
  4. dcousens commented at 4:32 AM on April 12, 2017: contributor

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

  5. jtimon force-pushed on Apr 12, 2017
  6. jtimon force-pushed on Apr 12, 2017
  7. practicalswift commented at 7:24 AM on April 12, 2017: contributor

    concept ACK

    Very good!

  8. fanquake added the label Refactoring on Apr 12, 2017
  9. fanquake added the label Scripts and tools on Apr 12, 2017
  10. jtimon force-pushed on Apr 12, 2017
  11. jtimon renamed this:
    scripted-diff: sed -i 's/BOOST_FOREACH(\(.*\),/for (\1 :/' ./src/qt/*.cpp
    scripted-diff: Remove BOOST_FOREACH
    on Apr 12, 2017
  12. jtimon force-pushed on Apr 12, 2017
  13. jtimon renamed this:
    scripted-diff: Remove BOOST_FOREACH
    scripted-diff: #include <boost/foreach.hpp>
    on Apr 13, 2017
  14. jtimon renamed this:
    scripted-diff: #include <boost/foreach.hpp>
    scripted-diff: Remove #include <boost/foreach.hpp>
    on Apr 13, 2017
  15. jtimon commented at 12:15 AM on April 13, 2017: contributor

    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 https://github.com/bitcoin/bitcoin/pull/10193/commits/cc40182b533dbe6282e7c6cb8d2dae61f9a7ead4 I tried https://github.com/bitcoin/bitcoin/pull/10193/commits/eb83e88d55cd3a4b230d9c99e6d8a97135167c74 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.

  16. jtimon cross-referenced this on Apr 13, 2017 from issue Include cleanup by jtimon
  17. in src/reverse_iterator.hpp:23 in c1b4089bba outdated 
      18 | +    
  19 | +public:
  20 | +    reverse_range(const T &x) : x(x) {}
  21 | +    
  22 | +    auto begin() const -> decltype(this->x.rbegin())
  23 | +    {
    dcousens commented at 12:41 AM on April 13, 2017:

    is the decltype even needed?

    jtimon commented at 12:09 PM on April 17, 2017:

    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.

    dcousens commented at 1:10 PM on April 17, 2017:

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

  18. in src/init.cpp:604 in c1b4089bba outdated 
     600 | @@ -601,7 +601,7 @@ void CleanupBlockRevFiles()
 601 |      // keeping a separate counter.  Once we hit a gap (or if 0 doesn't exist)
 602 |      // start removing block files.
 603 |      int nContigCounter = 0;
 604 | -    BOOST_FOREACH(const PAIRTYPE(std::string, fs::path)& item, mapBlockFiles) {
 605 | +    for (const PAIRTYPE(std::string, fs::path)& item : mapBlockFiles) {
    tjps commented at 7:33 AM on April 13, 2017:

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

  19. gmaxwell commented at 7:32 PM on April 13, 2017: contributor

    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.

  20. MarcoFalke commented at 7:58 PM on April 13, 2017: member

    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.

  21. fanquake added this to the "In progress" column in a project
  22. jtimon force-pushed on Apr 17, 2017
  23. jtimon commented at 7:29 PM on April 17, 2017: contributor

    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

  24. jtimon force-pushed on Apr 17, 2017
  25. jtimon cross-referenced this on May 29, 2017 from issue Util: Remove redundant calls to argsGlobal.IsArgSet() by jtimon
  26. fanquake cross-referenced this on Jun 1, 2017 from issue remove the PAIRTYPE macro by benma
  27. jtimon force-pushed on Jun 1, 2017
  28. jtimon cross-referenced this on Jun 1, 2017 from issue scripted-diff: Remove BOOST_FOREACH, Q_FOREACH and PAIRTYPE by jtimon
  29. jtimon commented at 5:26 PM on June 1, 2017: contributor

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

  30. jtimon force-pushed on Jun 1, 2017
  31. jtimon force-pushed on Jun 2, 2017
  32. jtimon commented at 1:35 AM on June 2, 2017: contributor

    Needed rebase, see #10502 (comment)

  33. sipa cross-referenced this on Jun 4, 2017 from issue scripted-diff: Use the C++11 keyword nullptr to denote the pointer literal instead of the macro NULL by practicalswift
  34. jtimon force-pushed on Jun 5, 2017
  35. jtimon commented at 8:53 PM on June 5, 2017: contributor

    Needed rebase. Also, it seems I solved the reverse iterator problem in prevector_tests.cpp thanks to @theuni 's suggestion.

    Note: cfields thinks I should be doing what I've done on prevector_tests everywhere, so it's interesting if this passes the tests

  36. sipa commented at 1:07 AM on June 14, 2017: member

    Needs rebase.

  37. jtimon force-pushed on Jun 14, 2017
  38. jtimon commented at 3:36 AM on June 14, 2017: contributor

    Rebased, we still don't know if the fix @theuni suggested is needed only needed on prevector_tests.cpp as I though or everywhere as he thinks.

    If he is right, ideally all 5 lines changed in https://github.com/bitcoin/bitcoin/pull/10193/commits/ad087bd496e46b4ae93b96089c0d609f183d3e4d (aka 'scripted-diff: Remove BOOST_REVERSE_FOREACH' for when the rebase) deserve their own test that fails with that very commit.

    Otherwise I think this is ready to merge squashes aside. Hopefully this is now much easier to review once the simpler yet most disruptive parts have been merged with #10502.

  39. theuni commented at 5:07 AM on June 14, 2017: member

    Concept ACK, but NACK until reverse_range/reverse_iterator have their own tests as @jtimon mentioned above. I believe that the current usage is broken due to scope issues with reverse_iterate in a range-based-for. A language lawyer may prove otherwise, but it's unnecessarily risky without tests to back it up.

  40. jtimon commented at 3:03 PM on June 14, 2017: contributor

    I don't mean writting their own test for reverse_range/reverse_iterator, at that point I think I'd rather just delete it.

    What I mean is that if the 5 changes in https://github.com/bitcoin/bitcoin/commit/ad087bd496e46b4ae93b96089c0d609f183d3e4d are incorrect (as you think they are and unlike those in https://github.com/bitcoin/bitcoin/pull/10193/commits/e01c26c1f81a6ba5949e8292e42979e26be971aa ) then it is unfortunate that no test catches the at least 5 errors that this PR is supposedly introducing.

  41. in src/.clang-format:26 in 9c20ce17ea outdated 
      22 | @@ -23,7 +23,7 @@ ContinuationIndentWidth: 4
  23 |  Cpp11BracedListStyle: true
  24 |  DerivePointerAlignment: false
  25 |  DisableFormat:   false
  26 | -ForEachMacros:   [ foreach, Q_FOREACH, BOOST_FOREACH, BOOST_REVERSE_FOREACH ]
  27 | +ForEachMacros:   [ foreach ]
    MarcoFalke commented at 8:57 PM on June 18, 2017:

    Is this macro still used?

    jtimon commented at 9:45 PM on June 18, 2017:

    I think it was never used but it was there from the beginning ( https://github.com/jtimon/bitcoin/commit/2887bffcfdc138f53c60091ab2906790971c9af5 ). @sipa do you remember why we add it? Perhaps we can Remove the full ForEachMacro line ?

  42. jtimon commented at 9:47 PM on June 18, 2017: contributor

    TODO As pointed out on IRC by @theuni it seems we neither need to do the same as in prevector_tests everywhere, it is not even needed in prevector_tests.cpp itself with a small fix in prevector. Thanks again.

  43. jtimon force-pushed on Jun 19, 2017
  44. jtimon commented at 12:37 AM on June 19, 2017: contributor

    Updated with @theuni 's latest proposed fix and fully removing ForEachMacros from clang-format following @MarcoFalke 's hint.

  45. in src/Makefile.am:127 in 9bc92b1182 outdated 
     123 | @@ -124,6 +124,7 @@ BITCOIN_CORE_H = \
 124 |    pow.h \
 125 |    protocol.h \
 126 |    random.h \
 127 | +  reverse_iterator.hpp \
    theuni commented at 10:36 PM on June 19, 2017:

    nit: please rename to reverse_iterator.h to match the other headers

  46. theuni commented at 10:40 PM on June 19, 2017: member

    NACK retracted after some local experimentation. This is effectively tested in the prevector tests, which now work as expected.

    utACK other than the header name nit.

  47. in src/reverse_iterator.hpp:1 in aab5e9c8bb outdated 
       0 | @@ -0,0 +1,39 @@
   1 | +// Taken from https://gist.github.com/arvidsson/7231973
    MarcoFalke commented at 7:40 AM on June 20, 2017:

    Can we just use this here? I couldn't find that this is MIT licensed.

    jtimon commented at 1:38 AM on June 22, 2017:

    I assumed it is public domain, but perhaps we should ask the author? ping @arvidsson

    arvidsson commented at 12:55 PM on June 22, 2017:

    Yeah, it's public domain.

    practicalswift commented at 1:24 PM on June 22, 2017:

    Thanks @arvidsson!

  48. Introduce src/reverse_iterator.hpp and include it...
    ...where it will be needed

Taken from https://gist.github.com/arvidsson/7231973 with small
modifications to fit the bitcoin core project
    300851ec16
  49. Fix const_reverse_iterator constructor (pass const ptr) 33aed5bf89
  50. scripted-diff: Remove BOOST_REVERSE_FOREACH
    -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-
    3eff827f89
  51. scripted-diff: Remove #include <boost/foreach.hpp>
    -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-
    5995735c5b
  52. clang-format: Delete ForEachMacros b1268a19d0
  53. jtimon force-pushed on Jun 22, 2017
  54. jtimon commented at 1:49 AM on June 22, 2017: contributor

    Fixed @theuni 's nit.

  55. sipa commented at 8:09 AM on June 22, 2017: member

    utACK b1268a19d0b80401339ede2188abbd389f8d7fb0

  56. practicalswift commented at 8:17 AM on June 22, 2017: contributor

    utACK b1268a1 modulo licensing approval from @arvidsson :-)

  57. fanquake commented at 12:56 AM on June 28, 2017: member

    ACK b1268a1

    ~140 Boost includes left, with the majority being in /test code.

  58. laanwj commented at 4:05 PM on July 4, 2017: member

    utACK b1268a1

  59. laanwj merged this on Jul 4, 2017
  60. laanwj closed this on Jul 4, 2017
  61. laanwj referenced this in commit 6dbcc74a0e on Jul 4, 2017
  62. jtimon deleted the branch on Jul 4, 2017
  63. paveljanik commented at 12:02 PM on July 8, 2017: contributor

    This PR brought few shadowing warnings:

    ./reverse_iterator.h:20:22: warning: declaration shadows a field of 'reverse_range<T>' [-Wshadow]
  64. jtimon commented at 5:51 PM on July 8, 2017: contributor

    Sorry about that. It seems I don't get those warnings locally?

  65. paveljanik commented at 4:30 AM on July 10, 2017: contributor

    @jtimon You have to build with -Wshadow.

  66. jtimon commented at 6:04 PM on July 10, 2017: contributor

    @paveljanik any reason why that's not the default if we want to comply with it?

  67. paveljanik commented at 6:15 PM on July 10, 2017: contributor

    Long story short: compilers are evolving and support this option differently. Sometimes old versions warn about correct code etc.

  68. jtimon cross-referenced this on Jul 13, 2017 from issue TODO for release notes 0.15.0 by MarcoFalke
  69. fanquake moved this from the "In progress" to the "Done" column in a project
  70. paveljanik cross-referenced this on Jul 15, 2017 from issue Rename member field according to the style guide by paveljanik
  71. MarcoFalke referenced this in commit e526ca6284 on Aug 9, 2017
  72. sickpig cross-referenced this on Apr 5, 2019 from issue Reverse iterator by sickpig
  73. PastaPastaPasta referenced this in commit 01b1e9abd3 on Jul 17, 2019
  74. PastaPastaPasta referenced this in commit dad5b65304 on Jul 17, 2019
  75. PastaPastaPasta referenced this in commit 28f20a0484 on Jul 17, 2019
  76. PastaPastaPasta referenced this in commit 4cf0429c1c on Jul 17, 2019
  77. PastaPastaPasta referenced this in commit 501c09cd02 on Jul 17, 2019
  78. PastaPastaPasta referenced this in commit 02cd116eb9 on Jul 17, 2019
  79. PastaPastaPasta referenced this in commit a308f1bf38 on Jul 17, 2019
  80. PastaPastaPasta referenced this in commit 80a73b3ae9 on Jul 17, 2019
  81. PastaPastaPasta referenced this in commit 88044d1077 on Jul 17, 2019
  82. PastaPastaPasta referenced this in commit 59ac58126b on Jul 17, 2019
  83. PastaPastaPasta referenced this in commit abbe483dff on Jul 23, 2019
  84. PastaPastaPasta referenced this in commit e06f673c67 on Jul 23, 2019
  85. PastaPastaPasta referenced this in commit 0b43955efd on Jul 23, 2019
  86. PastaPastaPasta referenced this in commit 8835d90a90 on Jul 23, 2019
  87. PastaPastaPasta referenced this in commit 0ad144bc50 on Jul 23, 2019
  88. PastaPastaPasta referenced this in commit 0fe9f757ec on Jul 24, 2019
  89. PastaPastaPasta referenced this in commit c123e10cc8 on Jul 24, 2019
  90. PastaPastaPasta referenced this in commit d6d462fd7b on Jul 24, 2019
  91. PastaPastaPasta referenced this in commit 96897e51a9 on Jul 24, 2019
  92. PastaPastaPasta referenced this in commit fa2cd234b2 on Jul 24, 2019
  93. PastaPastaPasta referenced this in commit 3f4130a57a on Sep 8, 2019
  94. PastaPastaPasta referenced this in commit 86a8b30637 on Sep 8, 2019
  95. PastaPastaPasta referenced this in commit 3e7ff33068 on Sep 8, 2019
  96. barrystyle referenced this in commit 82c6afdded on Jan 22, 2020
  97. barrystyle referenced this in commit 2673fa2b18 on Jan 22, 2020
  98. barrystyle referenced this in commit fac9c8c486 on Jan 22, 2020
  99. barrystyle referenced this in commit 1d06791e84 on Jan 22, 2020
  100. barrystyle referenced this in commit 8c4cd4379c on Jan 22, 2020
  101. barrystyle referenced this in commit 47b1eabed7 on Jan 22, 2020
  102. str4d cross-referenced this on Nov 23, 2020 from issue Backport Boost removal PRs by str4d
  103. zkbot referenced this in commit 77c2a5f810 on Dec 4, 2020
  104. bitcoin locked this on Sep 8, 2021
Contributors
jtimondcousenspracticalswifttjpsgmaxwellMarcoFalkesipatheuniarvidssonfanquakelaanwjpaveljanik
Labels
RefactoringScripts and tools
Linked (view graph)
#9889 TODO for release notes 0.15.0#10118 Util: Remove redundant calls to argsGlobal.IsArgSet()#10181 Include cleanup #10189 devtools/net: add a verifier for scriptable changes. Use it to make CNode::id private.#10483 scripted-diff: Use the C++11 keyword nullptr to denote the pointer literal instead of the macro NULL#10497 remove the PAIRTYPE macro#10502 scripted-diff: Remove BOOST_FOREACH, Q_FOREACH and PAIRTYPE#10835 Rename member field according to the style guide
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-05-19 06:54 UTC