refactor: Drop boost/algorithm/string/replace.hpp dependency #25803

1. 
   hebasto commented at 10:57 am on August 8, 2022: member

   A new implementation of the ReplaceAll() seems enough for all of our purposes.
2. test: Add test case for `ReplaceAll()` function 857526e8cb
3. refactor: Drop `boost/algorithm/string/replace.hpp` dependency fea75ad3ca
4. fanquake added the label Refactoring on Aug 8, 2022
5. 
   Sjors commented at 4:42 pm on August 8, 2022: member

   Concept ACK

   ReplaceAll() is currently used for handling -blocknotify, -walletnotify,-alertnotify and escaping slashes in -torpassword.

6. 
   luke-jr commented at 1:59 am on August 10, 2022: member

   What do we gain from this?
7. 
   hebasto commented at 8:06 am on August 10, 2022: member

   @luke-jr

   What do we gain from this?

   As the title said:

   Drop boost/algorithm/string/replace.hpp dependency

8. 
   Sjors commented at 8:35 am on August 10, 2022: member

   Alternatively: reducing the number of Boost includes by 10% :-)
9. 
   fanquake commented at 11:07 am on August 10, 2022: member

   ACK

   What do we gain from this?

   Using the standard library over a third party dependency.

10. 
    adam2k commented at 5:43 pm on August 10, 2022: none

    ACK Tested fea75ad3caa29972db32d3ce7e0fe125ec77a0eb

    This is anecdotal, but on an M1 MacBook Pro 14" 2021 (8 core CPU) there is a slight performance boost (no pun intended 😅)

    On master make check 289.85s user 24.91s system 55% cpu 9:24.87 total

    On fea75ad3caa29972db32d3ce7e0fe125ec77a0eb make check 276.10s user 22.31s system 48% cpu 10:11.89 total

11. 
    MarcoFalke commented at 5:56 pm on August 10, 2022: member

    Not that it matters for our use cases, but I’d suspect that regex is slower than string-replace in a microbench.
12. theStack approved
13. 
    theStack commented at 6:55 pm on August 15, 2022: contributor

    Code-review ACK fea75ad3caa29972db32d3ce7e0fe125ec77a0eb

    The idea of using regexes for the task of a simple replace-all substitution seemed excessive to me at first, but IMHO it’s worth it to get rid of another Boost dependency.

    nit: Untested, but IIUC, passing the flag match_not_null would be an alternative to the manual check whether the search string is empty? https://en.cppreference.com/w/cpp/regex/match_flag_type

14. 
    kristapsk commented at 9:33 pm on August 15, 2022: contributor

    Concept ACK on getting rid of external dependency.
15. 
    luke-jr commented at 11:26 pm on August 15, 2022: member

    The only thing worse than a dependency, is a bundled reimplementation. Especially if the implementation is worse.
16. fanquake merged this on Aug 16, 2022
17. fanquake closed this on Aug 16, 2022

    ---

18. hebasto deleted the branch on Aug 16, 2022
19. in src/util/string.cpp:14 in fea75ad3ca

    13-void ReplaceAll(std::string& in_out, std::string_view search, std::string_view substitute)
    14+void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute)
    15 {
    16-    boost::replace_all(in_out, search, substitute);
    17+    if (search.empty()) return;
    18+    in_out = std::regex_replace(in_out, std::regex(std::move(search)), substitute);
    

    ---

    ---

    

    MarcoFalke commented at 8:44 am on August 16, 2022:

    you can’t move a const value. This is a no-op. Even if it wasn’t const, this would still be a no-op as there is no constructor taking a non-const ref

    ---

    

    MarcoFalke commented at 12:16 pm on August 19, 2022:

    #25872
20. sidhujag referenced this in commit c9306ffbbe on Aug 16, 2022
21. Fabcien referenced this in commit c682e1d7f6 on Jun 3, 2023
22. bitcoin locked this on Aug 19, 2023

Contributors
hebasto Sjors luke-jr fanquake adam2k MarcoFalke theStack kristapsk

Labels
Refactoring