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:

    0sed -i 's/BOOST_FOREACH *(\(.*\),/for (\1 :/' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ./src/*/*/*.h ./src/*/*/*.cpp ;
1sed -i 's/BOOST_REVERSE_FOREACH(\(.*\), \(.*\))/for (\1 : reverse_iterate(\2))/' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ;
2sed -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 force-pushed on Apr 12, 2017
  3. 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?
  4. jtimon force-pushed on Apr 12, 2017
  5. jtimon force-pushed on Apr 12, 2017
  6. practicalswift commented at 7:24 am on April 12, 2017: contributor

    concept ACK

    Very good!

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

  15. 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 0: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
  16. 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?
  17. 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.
  18. 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.
  19. fanquake added this to the "In progress" column in a project
  20. jtimon force-pushed on Apr 17, 2017
  21. 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

  22. jtimon force-pushed on Apr 17, 2017
  23. jtimon force-pushed on Jun 1, 2017
  24. 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
  25. jtimon force-pushed on Jun 1, 2017
  26. jtimon force-pushed on Jun 2, 2017
  27. jtimon commented at 1:35 am on June 2, 2017: contributor
    Needed rebase, see #10502 (comment)
  28. jtimon force-pushed on Jun 5, 2017
  29. 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

  30. sipa commented at 1:07 am on June 14, 2017: member
    Needs rebase.
  31. jtimon force-pushed on Jun 14, 2017
  32. 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.

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

  35. 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 ?
  36. 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.
  37. jtimon force-pushed on Jun 19, 2017
  38. jtimon commented at 0: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.
  39. 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
  40. 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.

  41. 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!
  42. 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
  43. Fix const_reverse_iterator constructor (pass const ptr) 33aed5bf89
  44. 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
  45. 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
  46. clang-format: Delete ForEachMacros b1268a19d0
  47. jtimon force-pushed on Jun 22, 2017
  48. jtimon commented at 1:49 am on June 22, 2017: contributor
    Fixed @theuni ’s nit.
  49. sipa commented at 8:09 am on June 22, 2017: member
    utACK b1268a19d0b80401339ede2188abbd389f8d7fb0
  50. practicalswift commented at 8:17 am on June 22, 2017: contributor
    utACK b1268a1 modulo licensing approval from @arvidsson :-)
  51. fanquake commented at 0:56 am on June 28, 2017: member

    ACK b1268a1

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

  52. laanwj commented at 4:05 pm on July 4, 2017: member
    utACK b1268a1
  53. laanwj merged this on Jul 4, 2017
  54. laanwj closed this on Jul 4, 2017
  55. laanwj referenced this in commit 6dbcc74a0e on Jul 4, 2017
  56. jtimon deleted the branch on Jul 4, 2017
  57. paveljanik commented at 12:02 pm on July 8, 2017: contributor

    This PR brought few shadowing warnings:

    0./reverse_iterator.h:20:22: warning: declaration shadows a field of 'reverse_range<T>' [-Wshadow]
  58. jtimon commented at 5:51 pm on July 8, 2017: contributor
    Sorry about that. It seems I don’t get those warnings locally?
  59. paveljanik commented at 4:30 am on July 10, 2017: contributor
    @jtimon You have to build with -Wshadow.
  60. 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?
  61. 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.
  62. fanquake moved this from the "In progress" to the "Done" column in a project
  63. MarcoFalke referenced this in commit e526ca6284 on Aug 9, 2017
  64. PastaPastaPasta referenced this in commit 01b1e9abd3 on Jul 17, 2019
  65. PastaPastaPasta referenced this in commit dad5b65304 on Jul 17, 2019
  66. PastaPastaPasta referenced this in commit 28f20a0484 on Jul 17, 2019
  67. PastaPastaPasta referenced this in commit 4cf0429c1c on Jul 17, 2019
  68. PastaPastaPasta referenced this in commit 501c09cd02 on Jul 17, 2019
  69. PastaPastaPasta referenced this in commit 02cd116eb9 on Jul 17, 2019
  70. PastaPastaPasta referenced this in commit a308f1bf38 on Jul 17, 2019
  71. PastaPastaPasta referenced this in commit 80a73b3ae9 on Jul 17, 2019
  72. PastaPastaPasta referenced this in commit 88044d1077 on Jul 17, 2019
  73. PastaPastaPasta referenced this in commit 59ac58126b on Jul 17, 2019
  74. PastaPastaPasta referenced this in commit abbe483dff on Jul 23, 2019
  75. PastaPastaPasta referenced this in commit e06f673c67 on Jul 23, 2019
  76. PastaPastaPasta referenced this in commit 0b43955efd on Jul 23, 2019
  77. PastaPastaPasta referenced this in commit 8835d90a90 on Jul 23, 2019
  78. PastaPastaPasta referenced this in commit 0ad144bc50 on Jul 23, 2019
  79. PastaPastaPasta referenced this in commit 0fe9f757ec on Jul 24, 2019
  80. PastaPastaPasta referenced this in commit c123e10cc8 on Jul 24, 2019
  81. PastaPastaPasta referenced this in commit d6d462fd7b on Jul 24, 2019
  82. PastaPastaPasta referenced this in commit 96897e51a9 on Jul 24, 2019
  83. PastaPastaPasta referenced this in commit fa2cd234b2 on Jul 24, 2019
  84. PastaPastaPasta referenced this in commit 3f4130a57a on Sep 8, 2019
  85. PastaPastaPasta referenced this in commit 86a8b30637 on Sep 8, 2019
  86. PastaPastaPasta referenced this in commit 3e7ff33068 on Sep 8, 2019
  87. barrystyle referenced this in commit 82c6afdded on Jan 22, 2020
  88. barrystyle referenced this in commit 2673fa2b18 on Jan 22, 2020
  89. barrystyle referenced this in commit fac9c8c486 on Jan 22, 2020
  90. barrystyle referenced this in commit 1d06791e84 on Jan 22, 2020
  91. barrystyle referenced this in commit 8c4cd4379c on Jan 22, 2020
  92. barrystyle referenced this in commit 47b1eabed7 on Jan 22, 2020
  93. zkbot referenced this in commit 77c2a5f810 on Dec 4, 2020
  94. MarcoFalke locked this on Sep 8, 2021


jtimon dcousens practicalswift tjps gmaxwell MarcoFalke sipa theuni arvidsson fanquake laanwj paveljanik

Labels
Refactoring Scripts and tools

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: 2024-11-24 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me