refactor: Drop boost/algorithm/string/replace.hpp dependency
#25803
Conversation
|
Concept ACK
|
|
What do we gain from this? |
As the title said:
|
|
Alternatively: reducing the number of Boost includes by 10% :-) |
|
ACK
Using the standard library over a third party dependency. |
|
Not that it matters for our use cases, but I'd suspect that regex is slower than string-replace in a microbench. |
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
Code-review ACK fea75ad
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
|
Concept ACK on getting rid of external dependency. |
|
The only thing worse than a dependency, is a bundled reimplementation. Especially if the implementation is worse. |
| { | ||
| boost::replace_all(in_out, search, substitute); | ||
| if (search.empty()) return; | ||
| in_out = std::regex_replace(in_out, std::regex(std::move(search)), substitute); |
There was a problem hiding this comment.
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
Summary: > test: Add test case for `ReplaceAll()` function > refactor: Drop `boost/algorithm/string/replace.hpp` dependency This is a backport of [[bitcoin/bitcoin#25803 | core#25803]] with a `std::move` removed change (see comment by MarcoFalke bitcoin/bitcoin@fea75ad#r946498588) Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13968
8 participants
A new implementation of the
ReplaceAll()seems enough for all of our purposes.