Remove the boost/algorithm/string/predicate.hpp dependency #13656

l2a5b1 · 2018-07-13T14:45:15Z

This pull request removes the boost/algorithm/string/predicate.hpp dependency from the project.

To replace the the predicate.hpp dependency from the project the function calls to boost::algorithm::starts_with and boost::algorithm::ends_with have been replaced with respectively C++11's std::basic_string::front and std::basic_string::back function calls.

Refactors that were not required, but have been done anyways:

The Boost function all was implicitly made available via the predicate.hpp header. Instead of including the appropriate header, function calls to all have been replaced with function calls to std::all_of.
The boost::algorithm::is_digit predicate has been replaced with a custom IsDigit function that is locale independent and ASCII deterministic.

maflcko · 2018-07-13T15:05:48Z

utACK 6285655d9592bf684f2ddc69871456fb3628b8a6

Empact · 2018-07-13T16:22:54Z

utACK 6285655

fanquake · 2018-07-15T12:52:21Z

Concept ACK

After this change there are only a few other uses of starts_with and ends_with left in the code base, all in core_read.cpp and src/wallet/rpcdump.cpp. @251labs Did you want to update this PR to remove all the remaining instances in one go?

If you do, please also remove <boost/algorithm/string/predicate.hpp> from lint-includes.sh

starts_with

bitcoin/src/core_read.cpp

Line 57 in 8803c91

    
           (boost::algorithm::starts_with(*w, "-") && all(std::string(w->begin()+1, w->end()), boost::algorithm::is_digit())))

bitcoin/src/core_read.cpp

Line 63 in 8803c91

    
           else if (boost::algorithm::starts_with(*w, "0x") && (w->begin()+2 != w->end()) && IsHex(std::string(w->begin()+2, w->end())))

bitcoin/src/core_read.cpp

Line 69 in 8803c91

    
           else if (w->size() >= 2 && boost::algorithm::starts_with(*w, "'") && boost::algorithm::ends_with(*w, "'"))

bitcoin/src/wallet/rpcdump.cpp

Line 588 in 8803c91

if (boost::algorithm::starts_with(vstr[nStr], "#"))

bitcoin/src/wallet/rpcdump.cpp

Line 594 in 8803c91

if (boost::algorithm::starts_with(vstr[nStr], "label=")) {

ends_with

bitcoin/src/core_read.cpp

Line 69 in 8803c91

    
           else if (w->size() >= 2 && boost::algorithm::starts_with(*w, "'") && boost::algorithm::ends_with(*w, "'"))

Empact · 2018-07-15T13:07:23Z

Could follow @fanquake's suggestion for just the single-character checks.

l2a5b1 · 2018-07-15T23:02:50Z

Thanks @fanquake and @Empact. I kept the PR as small as possible for ease of review, but I am happy to have a go at the remaining instances.

DrahtBot · 2018-07-16T11:23:25Z

Note to reviewers: This pull request conflicts with the following ones:

Remove the boost/algorithm/string/case_conv.hpp dependency #13671 (Remove the boost/algorithm/string/case_conv.hpp dependency by 251Labs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Empact · 2018-07-16T19:34:50Z

@251labs depends on circumstances and the fix, but dealing with all identical cases in a single PR can help minimize review effort to avoid splitting consideration and discussion over multiple PRs.

Even better would be to lint against single-char starts_with/ends_with, to prevent future introduction, but I doubt that's worthwhile in this case.

l2a5b1 · 2018-07-17T14:11:27Z

Thanks for the clarification @Empact. I have updated the PR and changed the title and description accordingly.

Empact · 2018-07-17T15:04:25Z

src/utilstrencodings.cpp

@@ -425,6 +425,11 @@ int atoi(const std::string& str)
    return atoi(str.c_str());
 }

+bool IsDigit(char c)


nit: constexpr

Nice one @Empact, addressed in 2a02b4a.

Empact · 2018-07-17T15:30:07Z

nit: There are 5 places in utilstrencoding.cpp where IsDigit could be applied. Could do that here or in a follow-up but here makes sense to me.

Note the other possible uses are in included projects, e..g leveldb, tinyformat, which shouldn't be changed.

l2a5b1 · 2018-07-18T17:03:43Z

Thanks again @Empact , I have addressed your feedback in 2a02b4a.

Empact · 2018-07-18T18:17:12Z

utACK 146793c

nit: could squash

l2a5b1 · 2018-07-19T21:11:41Z

Thanks @Empact, I am happy to squash this.

Just wanted to let you know that 2a02b4a is an atomic commit on its own. Strictly speaking the work in 2a02b4a was not required to drop the predicate dependency in 146793c. Maybe a squashed commit would be confusing from that POV (particularly in a limited blame- or history view).

Either way, I am happy to squash this. Please let me know what is preferred and I will take it from there :)

Empact · 2018-07-20T17:36:46Z

Could go either way, but IMO neither is so complicated that they don't bear combination, and the latter motivates the former.

sipa · 2018-07-20T18:05:05Z

utACK 146793caa9403310ae44d1f7fa54dc077d4b2d39

maflcko · 2018-07-20T20:10:53Z

src/core_read.cpp

        {
            // Number
            int64_t n = atoi64(*w);
            result << n;
        }
-        else if (boost::algorithm::starts_with(*w, "0x") && (w->begin()+2 != w->end()) && IsHex(std::string(w->begin()+2, w->end())))
+        else if (w->substr(0,2) == "0x" && (w->begin()+2 < w->end()) && IsHex(std::string(w->begin()+2, w->end())))


unrelated nit: could use std::string::size > 2 here

maflcko · 2018-07-20T20:12:07Z

utACK 146793caa9403310ae44d1f7fa54dc077d4b2d39

This is a squashed commit that squashes the following commits: This commit removes the `boost/algorithm/string/predicate.hpp` dependenc from the project by replacing the function calls to `boost::algorithm::starts_with` `boost::algorithm::ends_with` and `all` with respectively C++11' `std::basic_string::front`, `std::basic_string::back`, `std::all_of` function calls This commit replaces `boost::algorithm::is_digit` with a locale independent isdigi function, because the use of the standard library's `isdigit` and `std::isdigit functions is discoraged in the developer notes

l2a5b1 · 2018-07-22T20:45:23Z

Thanks for the reviews! Not sure if it is appreciated if nits are addressed following utACKs, but e3245f2 addresses @MarcoFalke's nit #13656 (comment); squashes individual commits per @Empact's feedback; and is rebased on master (0a34593).

Empact · 2018-07-23T00:46:24Z

utACK e3245f2

maflcko · 2018-07-23T14:11:46Z

re-utACK e3245f2

e3245f2 Removes Boost predicate.hpp dependency (251) Pull request description: This pull request removes the `boost/algorithm/string/predicate.hpp` dependency from the project. To replace the the `predicate.hpp` dependency from the project the function calls to `boost::algorithm::starts_with` and `boost::algorithm::ends_with` have been replaced with respectively C++11's `std::basic_string::front` and `std::basic_string::back` function calls. Refactors that were not required, but have been done anyways: - The Boost function `all` was implicitly made available via the `predicate.hpp` header. Instead of including the appropriate header, function calls to `all` have been replaced with function calls to `std::all_of`. - The `boost::algorithm::is_digit` predicate has been replaced with a custom `IsDigit` function that is locale independent and ASCII deterministic. Tree-SHA512: 22dda6adfb4d7ac0cabac8cc33e8fb8330c899805acc1ae4ede402c4b11ea75a399414b389dfaa3650d23b47f41351b4650077af9005d598fbe48d5277bdc320

Summary: e3245f2 Removes Boost predicate.hpp dependency (251) Pull request description: This pull request removes the `boost/algorithm/string/predicate.hpp` dependency from the project. To replace the the `predicate.hpp` dependency from the project the function calls to `boost::algorithm::starts_with` and `boost::algorithm::ends_with` have been replaced with respectively C++11's `std::basic_string::front` and `std::basic_string::back` function calls. Refactors that were not required, but have been done anyways: - The Boost function `all` was implicitly made available via the `predicate.hpp` header. Instead of including the appropriate header, function calls to `all` have been replaced with function calls to `std::all_of`. - The `boost::algorithm::is_digit` predicate has been replaced with a custom `IsDigit` function that is locale independent and ASCII deterministic. Tree-SHA512: 22dda6adfb4d7ac0cabac8cc33e8fb8330c899805acc1ae4ede402c4b11ea75a399414b389dfaa3650d23b47f41351b4650077af9005d598fbe48d5277bdc320 Backport of Core PR13656 bitcoin/bitcoin#13656 Test Plan: make check Undo the change to the linter script arc lint --everything Should output: Advice (BOOST_DEPENDENCY) Good job! A boost dependency has been removed. Good job! The Boost dependency "boost/algorithm/string/predicate.hpp" is no longer used. Please remove it from EXPECTED_BOOST_INCLUDES in <path>/bitcoin-abc/test/lint/lint-boost-dependencies.sh to make sure this dependency is not accidentally reintroduced. Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc, markblundeberg Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, markblundeberg Subscribers: markblundeberg Differential Revision: https://reviews.bitcoinabc.org/D4727

…dependency e3245f2 Removes Boost predicate.hpp dependency (251) Pull request description: This pull request removes the `boost/algorithm/string/predicate.hpp` dependency from the project. To replace the the `predicate.hpp` dependency from the project the function calls to `boost::algorithm::starts_with` and `boost::algorithm::ends_with` have been replaced with respectively C++11's `std::basic_string::front` and `std::basic_string::back` function calls. Refactors that were not required, but have been done anyways: - The Boost function `all` was implicitly made available via the `predicate.hpp` header. Instead of including the appropriate header, function calls to `all` have been replaced with function calls to `std::all_of`. - The `boost::algorithm::is_digit` predicate has been replaced with a custom `IsDigit` function that is locale independent and ASCII deterministic. Tree-SHA512: 22dda6adfb4d7ac0cabac8cc33e8fb8330c899805acc1ae4ede402c4b11ea75a399414b389dfaa3650d23b47f41351b4650077af9005d598fbe48d5277bdc320

fanquake added Refactoring P2P labels Jul 13, 2018

DrahtBot mentioned this pull request Jul 16, 2018

Remove the boost/algorithm/string/case_conv.hpp dependency #13671

Merged

l2a5b1 force-pushed the patch/remove_boost_predicate_from_netbase branch 4 times, most recently from e1c3061 to 522203e Compare July 17, 2018 12:22

l2a5b1 changed the title ~~Remove the boost/algorithm/string/predicate.hpp dependency from netbase.cpp~~ Remove the boost/algorithm/string/predicate.hpp dependency Jul 17, 2018

Empact reviewed Jul 17, 2018

View reviewed changes

l2a5b1 force-pushed the patch/remove_boost_predicate_from_netbase branch 2 times, most recently from cb90b20 to 146793c Compare July 18, 2018 16:15

maflcko reviewed Jul 20, 2018

View reviewed changes

DrahtBot added the Needs rebase label Jul 22, 2018

l2a5b1 force-pushed the patch/remove_boost_predicate_from_netbase branch from 146793c to ddd4e80 Compare July 22, 2018 19:22

l2a5b1 force-pushed the patch/remove_boost_predicate_from_netbase branch from ddd4e80 to e3245f2 Compare July 22, 2018 19:34

DrahtBot removed the Needs rebase label Jul 22, 2018

l2a5b1 mentioned this pull request Jul 24, 2018

Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency #13751

Closed

maflcko merged commit e3245f2 into bitcoin:master Jul 24, 2018

Bushstar mentioned this pull request Aug 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#358

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Remove the boost/algorithm/string/predicate.hpp dependency #13656

Remove the boost/algorithm/string/predicate.hpp dependency #13656

Uh oh!

Conversation

l2a5b1 commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 13, 2018

Uh oh!

Empact commented Jul 13, 2018

Uh oh!

fanquake commented Jul 15, 2018

starts_with

ends_with

Uh oh!

Empact commented Jul 15, 2018

Uh oh!

l2a5b1 commented Jul 15, 2018

Uh oh!

DrahtBot commented Jul 16, 2018

Uh oh!

Empact commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l2a5b1 commented Jul 17, 2018

Uh oh!

Empact Jul 17, 2018

Choose a reason for hiding this comment

Uh oh!

l2a5b1 Jul 18, 2018

Choose a reason for hiding this comment

Uh oh!

Empact commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l2a5b1 commented Jul 18, 2018

Uh oh!

Empact commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l2a5b1 commented Jul 19, 2018

Uh oh!

Empact commented Jul 20, 2018

Uh oh!

sipa commented Jul 20, 2018

Uh oh!

maflcko Jul 20, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 20, 2018

Uh oh!

l2a5b1 commented Jul 22, 2018

Uh oh!

Empact commented Jul 23, 2018

Uh oh!

maflcko commented Jul 23, 2018

Uh oh!

Uh oh!

l2a5b1 commented Jul 13, 2018 •

edited

Loading

Empact commented Jul 16, 2018 •

edited

Loading

Empact commented Jul 17, 2018 •

edited

Loading

Empact commented Jul 18, 2018 •

edited

Loading