refactor: remove use of boost::algorithm::replace_first #20067

pull theStack wants to merge 1 commits into bitcoin:master from theStack:20201003-get-rid-of-boost-replace_first changing 1 files +3 −3

theStack commented at 11:01 AM on October 3, 2020: member

As discussed in #19851 (https://github.com/bitcoin/bitcoin/pull/19851#issuecomment-685424702), this trivial PR substitutes the (only) use of boost::algorithm::replace_first by a direct implementation.
DrahtBot added the label Refactoring on Oct 3, 2020
practicalswift commented at 7:15 PM on October 4, 2020: contributor

Concept ACK

Every step towards a Boost free future counts :)

in src/core_read.cpp:43 in fadc23d266 outdated

  38 | @@ -40,8 +39,9 @@ CScript ParseScript(const std::string& s)
  39 |                  continue;
  40 |              mapOpNames[strName] = static_cast<opcodetype>(op);
  41 |              // Convenience: OP_ADD and just ADD are both recognized:
  42 | -            boost::algorithm::replace_first(strName, "OP_", "");

practicalswift commented at 7:18 PM on October 4, 2020:

Wouldn't find instead of rfind make the code match the previous replace_first version 100%?

theStack commented at 8:20 AM on October 5, 2020:

Indeed, to match the previous version 100% the code would look something like this:

    ...
    // Convenience: OP_ADD and just ADD are both recognized:
    auto pos = strName.find("OP_");
    if (pos != std::string::npos) {
        strName = strName.substr(0, pos) + strName.substr(pos+3);
        mapOpNames[strName] = static_cast<opcodetype>(op);
    }
    ...

That would also eliminate an "OP_" substring that is not at the start of strName. Don't know though if that really makes sense to match the previous version 100% (which did more than needed -- pos will always be either 0 or npos)? I guess it's a matter of taste and how strict a refactoring is defined.

MarcoFalke commented at 8:32 AM on October 5, 2020: member

mapOpNames wouldn't be written to if the name doesn't start with OP_ is this change wanted? Also, why can't this just use the stdlib remove_prefix? https://en.cppreference.com/w/cpp/string/basic_string_view/remove_prefix
theStack commented at 9:00 AM on October 5, 2020: member

mapOpNames wouldn't be written to if the name doesn't start with OP_ is this change wanted?

It is, the line above the comment isn't changed or removed by this PR: https://github.com/bitcoin/bitcoin/blob/875e1ccc9fe01e026e564dfd39a64d9a4b332a89/src/core_read.cpp#L41-L44

Also, why can't this just use the stdlib remove_prefix? https://en.cppreference.com/w/cpp/string/basic_string_view/remove_prefix

Nice idea, unfortunately only available since C++17...
MarcoFalke commented at 9:11 AM on October 5, 2020: member

Nice idea, unfortunately only available since C++17...

I think this refactor is low-prio enough, so that it can wait the 4 weeks until we have C++17
MarcoFalke commented at 6:05 PM on October 5, 2020: member

Nevermind, on a second look, the remove_prefix doesn't fit this use case
MarcoFalke closed this on Oct 5, 2020
MarcoFalke reopened this on Oct 5, 2020
luke-jr changes_requested
luke-jr commented at 6:11 PM on October 5, 2020: member

Concept NACK: Seems like a waste of time to review and fix. Boost isn't going anywhere any time soon, and that's okay.

This won't work with bare opcodes as-is.
theStack commented at 11:33 AM on October 6, 2020: member

Concept NACK: Seems like a waste of time to review and fix. Boost isn't going anywhere any time soon, and that's okay.

The reason I opened this PR was that it was actively proposed by regular contributors (see e.g. #19851 (comment), #19851 (comment)). I also saw quite a lot of merged PRs within the last months that try to get rid of boost. Can you elaborate more on why you think getting rid of Boost is not worthwile?

This won't work with bare opcodes as-is.

Given that with "bare opcodes" you mean "opcodes not starting with 'OP_'": Those are put into the dictionary before the condition that is introduced by this patch. (If that wouldn't work, the tests would of course also fail.)

in src/core_read.cpp:42 in fadc23d266 outdated

  38 | @@ -40,8 +39,9 @@ CScript ParseScript(const std::string& s)
  39 |                  continue;
  40 |              mapOpNames[strName] = static_cast<opcodetype>(op);
  41 |              // Convenience: OP_ADD and just ADD are both recognized:
  42 | -            boost::algorithm::replace_first(strName, "OP_", "");
  43 | -            mapOpNames[strName] = static_cast<opcodetype>(op);
  44 | +            if (strName.rfind("OP_", 0) == 0) { // strName starts with "OP_"

luke-jr commented at 6:06 PM on October 24, 2020:

            if (strName.compare(0, 3, "OP_") == 0) {  // strName starts with "OP_"

theStack commented at 11:30 AM on October 25, 2020:

Agreed that this is more readable, done.

theStack force-pushed on Oct 25, 2020
refactor: remove use of boost::algorithm::replace_first 6f4e393646
theStack force-pushed on Oct 25, 2020
theStack commented at 11:36 AM on October 25, 2020: member

Rebased on master and followed luke-jrs suggestion, using the compare() method for checking the OP_ prefix rather than the less readable rfind().
laanwj commented at 10:46 AM on November 19, 2020: member

Code review ACK 6f4e3936461d0893250e7684928339f6cedf4d0c
laanwj merged this on Nov 19, 2020
laanwj closed this on Nov 19, 2020
sidhujag referenced this in commit f8a3fab2dc on Nov 19, 2020
theStack deleted the branch on Dec 1, 2020
PastaPastaPasta referenced this in commit f8f01cccbe on Jun 27, 2021
PastaPastaPasta referenced this in commit ee970bfe48 on Jun 28, 2021
PastaPastaPasta referenced this in commit e3fb5fd83d on Jun 29, 2021
PastaPastaPasta referenced this in commit a58b0f10e1 on Jul 1, 2021
PastaPastaPasta referenced this in commit ac8fd2a1c5 on Jul 1, 2021
kittywhiskers referenced this in commit 5a12d1eb86 on Jul 15, 2021
kittywhiskers referenced this in commit 0b490a0de6 on Jul 15, 2021
PastaPastaPasta referenced this in commit 21307a3345 on Jul 15, 2021
PastaPastaPasta referenced this in commit 96cf458083 on Jul 15, 2021
kittywhiskers referenced this in commit 9af3760ae3 on Jul 16, 2021
UdjinM6 referenced this in commit 730a89f8ed on Jul 16, 2021
Fabcien referenced this in commit e0303815ca on Jan 6, 2022
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19851 refactor: Extract ParseOpCode from ParseScript