Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 1 commit into from
Nov 19, 2020
Merged

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

laanwj merged 1 commit into from
Nov 19, 2020

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Oct 3, 2020

As discussed in #19851 (#19851 (comment)), this trivial PR substitutes the (only) use of boost::algorithm::replace_first by a direct implementation.

@practicalswift
Copy link
Contributor

practicalswift commented Oct 4, 2020

Concept ACK

Every step towards a Boost free future counts :)

practicalswift
practicalswift reviewed Oct 4, 2020
View reviewed changes
src/core_read.cpp Outdated
Copy link
Contributor

@practicalswift practicalswift Oct 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@theStack theStack Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@maflcko
Copy link
Member

maflcko commented Oct 5, 2020 •
edited
Loading

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
Copy link
Contributor Author

theStack commented Oct 5, 2020

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:

bitcoin/src/core_read.cpp

Lines 41 to 44 in 875e1cc

mapOpNames[strName] = static_cast<opcodetype>(op);
// Convenience: OP_ADD and just ADD are both recognized:
boost::algorithm::replace_first(strName, "OP_", "");
mapOpNames[strName] = static_cast<opcodetype>(op);

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

@maflcko
Copy link
Member

maflcko commented Oct 5, 2020

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

@maflcko
Copy link
Member

maflcko commented Oct 5, 2020

Nevermind, on a second look, the remove_prefix doesn't fit this use case

@maflcko maflcko closed this Oct 5, 2020
@maflcko maflcko reopened this Oct 5, 2020
luke-jr
luke-jr suggested changes Oct 5, 2020
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

theStack commented Oct 6, 2020

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.)

luke-jr
luke-jr reviewed Oct 24, 2020
View reviewed changes
@theStack theStack force-pushed the 20201003-get-rid-of-boost-replace_first branch from fadc23d to 1a6b374 Compare October 25, 2020 11:28
@theStack theStack force-pushed the 20201003-get-rid-of-boost-replace_first branch from 1a6b374 to 6f4e393 Compare October 25, 2020 11:30
@theStack
Copy link
Contributor Author

theStack commented Oct 25, 2020

Rebased on master and followed luke-jrs suggestion, using the compare() method for checking the OP_ prefix rather than the less readable rfind().

@laanwj
Copy link
Member

laanwj commented Nov 19, 2020

Code review ACK 6f4e393

@laanwj laanwj merged commit fbb2bee into bitcoin:master Nov 19, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 19, 2020
@laanwj
Merge bitcoin#20067: refactor: remove use of boost::algorithm::replac…
f8a3fab 
…e_first

6f4e393 refactor: remove use of boost::algorithm::replace_first (Sebastian Falbesoner)

Pull request description:

  As discussed in bitcoin#19851 (bitcoin#19851 (comment)), this trivial PR substitutes the (only) use of `boost::algorithm::replace_first` by a direct implementation.

ACKs for top commit:
  laanwj:
    Code review ACK 6f4e393

Tree-SHA512: 2ef06498e19f864a4cbae10e8d1905e3440a2d1e8e5aae83de7597c23cdab92b4612d7fa1efbc49016e530debd127d1d50531c60ff159dbea0deaa8c836a2bfb
@theStack theStack deleted the 20201003-get-rid-of-boost-replace_first branch December 1, 2020 09:54
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#20067: refactor: remove use of boost::algorithm::replac…
ac8fd2a 
…e_first

6f4e393 refactor: remove use of boost::algorithm::replace_first (Sebastian Falbesoner)

Pull request description:

  As discussed in bitcoin#19851 (bitcoin#19851 (comment)), this trivial PR substitutes the (only) use of `boost::algorithm::replace_first` by a direct implementation.

ACKs for top commit:
  laanwj:
    Code review ACK 6f4e393

Tree-SHA512: 2ef06498e19f864a4cbae10e8d1905e3440a2d1e8e5aae83de7597c23cdab92b4612d7fa1efbc49016e530debd127d1d50531c60ff159dbea0deaa8c836a2bfb
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#20067: refactor: remove use of boost::algorithm::replac…
21307a3 
…e_first

6f4e393 refactor: remove use of boost::algorithm::replace_first (Sebastian Falbesoner)

Pull request description:

  As discussed in bitcoin#19851 (bitcoin#19851 (comment)), this trivial PR substitutes the (only) use of `boost::algorithm::replace_first` by a direct implementation.

ACKs for top commit:
  laanwj:
    Code review ACK 6f4e393

Tree-SHA512: 2ef06498e19f864a4cbae10e8d1905e3440a2d1e8e5aae83de7597c23cdab92b4612d7fa1efbc49016e530debd127d1d50531c60ff159dbea0deaa8c836a2bfb
kwvg added a commit to kwvg/dash that referenced this pull request Jul 16, 2021
@kwvg
merge bitcoin#20067: remove use of boost::algorithm::replace_first
9af3760
UdjinM6 added a commit to dashpay/dash that referenced this pull request Jul 16, 2021
@UdjinM6
Merge pull request #4246 from kittywhiskers/deboostification
730a89f 
merge bitcoin#13671, bitcoin#17405, bitcoin#18786, bitcoin#18792, bitcoin#20067: Deboostification
@barton2526 barton2526 mentioned this pull request Oct 15, 2021
Merged
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 6, 2022
@theStack @PiRK
refactor: remove use of boost::algorithm::replace_first
e030381 
Summary:
This is a backport of [[bitcoin/bitcoin#20067 | core#20067]]

I remove a trivial comment, to avoid having two comments on the previous lines.

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10768
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@luke-jr luke-jr luke-jr requested changes

@practicalswift practicalswift practicalswift left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@theStack @practicalswift @maflcko @laanwj @luke-jr @DrahtBot