[trivial] Removed one Boost usage and headers in util.cpp #10059

pull tjps wants to merge 1 commits into bitcoin:master from tjps:tjps_boost changing 1 files +1 −3

tjps commented at 12:44 AM on March 23, 2017: contributor

No description provided.
fanquake added the label Refactoring on Mar 23, 2017
[trivial] Removed one Boost usage and headers in util.cpp 374e26244d
fanquake commented at 1:27 AM on March 23, 2017: member

If your going to start removing specific Boost functions, I'd suggest looking over the codebase and seeing if it makes sense to remove all usages in one go, to save lots of similar pull requests. It also make review easier.

In this case, we only have two usages of ends_with, in core_read.cpp and netbase.cpp.

There are 5 usages of starts_with (the 16 in leveldb/ don't count) in bitcoind.cpp, core_read.cpp, netbase.cpp, wallet/rpcdump.cpp and util.cpp as you've removed here.

I'd suggest updating this PR to remove all usages of both. Then please update the PR title, and drop [trivial].
tjps commented at 1:48 AM on March 23, 2017: contributor

That's a good idea, I was originally just offloading unrelated changes in smaller commits for a larger commit I'm working on. Will update this with those other usages.

In the past I've been told that sweeping refactors (such as my for-each PR: #8801) are generally discouraged and to update things as they are touched. Obviously this isn't as large of a set of changes, so I'm still working on finding the right balance.

Thanks for the pointer.
fanquake commented at 2:47 AM on March 23, 2017: member

@tjps Yes large sweeping changes that touch many files, and disrupt many other pull requests at the same time, for little immediate benefit are discouraged. As you say, it's finding the right balance between hundreds of trivial refactors, and trying to change 100 files at a time.
in src/util.cpp:365 in 374e26244d
```
 361 | @@ -364,7 +362,7 @@ void ParseParameters(int argc, const char* const argv[])
 362 |          }
 363 |  #ifdef WIN32
 364 |          boost::to_lower(str);
 365 | -        if (boost::algorithm::starts_with(str, "/"))
 366 | +        if (str.size() > 0 && str[0] == '/')
```
laanwj commented at 9:01 AM on March 29, 2017:

The algorithm predicate way of writing things is much more modern C++. If we'd do this, we should just provide our own implementation of boost::algorithm::starts_with. But I don't think replacing that is urgent.

tjps commented at 5:01 AM on March 30, 2017:

I agree that not replacing it is urgent, this originally stemmed from a one-off from another patch I was working on. Due to @fanquake's suggestion I looked at replacing all usages, and found cases where there was more than a single-character prefix/postfix match happening.

I'm dropping this PR altogether, not worth the change.
tjps closed this on Mar 30, 2017
tjps deleted the branch on May 13, 2017
DrahtBot locked this on Sep 8, 2021

Contributors

Labels