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

l2a5b1 commented at 2:45 pm on July 13, 2018: contributor

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.

fanquake added the label Refactoring on Jul 13, 2018

fanquake added the label P2P on Jul 13, 2018

MarcoFalke commented at 3:05 pm on July 13, 2018: member

utACK 6285655d9592bf684f2ddc69871456fb3628b8a6

Empact commented at 4:22 pm on July 13, 2018: member

utACK 6285655

fanquake commented at 12:52 pm on July 15, 2018: member

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

https://github.com/bitcoin/bitcoin/blob/8803c9132a78d8182bd828a29f7051fc7688f934/src/core_read.cpp#L57 https://github.com/bitcoin/bitcoin/blob/8803c9132a78d8182bd828a29f7051fc7688f934/src/core_read.cpp#L63 https://github.com/bitcoin/bitcoin/blob/8803c9132a78d8182bd828a29f7051fc7688f934/src/core_read.cpp#L69 https://github.com/bitcoin/bitcoin/blob/8803c9132a78d8182bd828a29f7051fc7688f934/src/wallet/rpcdump.cpp#L588 https://github.com/bitcoin/bitcoin/blob/8803c9132a78d8182bd828a29f7051fc7688f934/src/wallet/rpcdump.cpp#L594

ends_with

https://github.com/bitcoin/bitcoin/blob/8803c9132a78d8182bd828a29f7051fc7688f934/src/core_read.cpp#L69

Empact commented at 1:07 pm on July 15, 2018: member

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

l2a5b1 commented at 11:02 pm on July 15, 2018: contributor

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 commented at 11:23 am on July 16, 2018: member

#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 commented at 7:34 pm on July 16, 2018: member

@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 force-pushed on Jul 17, 2018

l2a5b1 renamed this:
~~Remove the boost/algorithm/string/predicate.hpp dependency from netbase.cpp~~
Remove the boost/algorithm/string/predicate.hpp dependency
on Jul 17, 2018

l2a5b1 commented at 2:11 pm on July 17, 2018: contributor

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

in src/utilstrencodings.cpp:428 in 522203e95e outdated

424@@ -425,6 +425,11 @@ int atoi(const std::string& str)
425     return atoi(str.c_str());
426 }
427 
428+bool IsDigit(char c)

Empact commented at 3:04 pm on July 17, 2018:

nit: constexpr

l2a5b1 commented at 5:01 pm on July 18, 2018:

Nice one @empact, addressed in 2a02b4a.

Empact commented at 3:30 pm on July 17, 2018: member

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 force-pushed on Jul 18, 2018

l2a5b1 commented at 5:03 pm on July 18, 2018: contributor

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

Empact commented at 6:17 pm on July 18, 2018: member

utACK 146793c

nit: could squash

l2a5b1 commented at 9:11 pm on July 19, 2018: contributor

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 commented at 5:36 pm on July 20, 2018: member

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

sipa commented at 6:05 pm on July 20, 2018: member

utACK 146793caa9403310ae44d1f7fa54dc077d4b2d39

in src/core_read.cpp:64 in 146793caa9 outdated

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

MarcoFalke commented at 8:10 pm on July 20, 2018:

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

MarcoFalke commented at 8:12 pm on July 20, 2018: member

utACK 146793caa9403310ae44d1f7fa54dc077d4b2d39

fanquake added this to the "In progress" column in a project

DrahtBot added the label Needs rebase on Jul 22, 2018

l2a5b1 force-pushed on Jul 22, 2018

Removes Boost predicate.hpp dependency

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

e3245f2e7b

l2a5b1 force-pushed on Jul 22, 2018

l2a5b1 commented at 8:45 pm on July 22, 2018: contributor

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

DrahtBot removed the label Needs rebase on Jul 22, 2018

Empact commented at 0:46 am on July 23, 2018: member

utACK e3245f2

MarcoFalke commented at 2:11 pm on July 23, 2018: member

re-utACK e3245f2e7b6f98cda38a3806da854f7d513fec2f

MarcoFalke merged this on Jul 24, 2018

MarcoFalke closed this on Jul 24, 2018

MarcoFalke referenced this in commit 1211b15bf6 on Jul 24, 2018

fanquake moved this from the "In progress" to the "Done" column in a project

jasonbcox referenced this in commit f17f29733e on Dec 20, 2019

PastaPastaPasta referenced this in commit ac5739ca29 on Jul 17, 2020

PastaPastaPasta referenced this in commit ab829528ca on Jul 17, 2020

PastaPastaPasta referenced this in commit 2829b4d3b9 on Jul 17, 2020

MarcoFalke locked this on Sep 8, 2021

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

starts_with

ends_with