Split up util.cpp/h

laanwj commented at 3:11 pm on August 21, 2014: member

100% code movement, no functional changes. Split up util.cpp/h into:

string utilities (hex, base32, base64): no internal dependencies, no dependency on boost (apart from foreach)
money utilities (parsemoney, formatmoney)
time utilities (gettime*, sleep, format date):
and the rest (logging, argument parsing, config file parsing)

The latter is basically the environment and OS handling, and is stripped of all utility functions, so we may want to rename it to something else than util.cpp/h for clarity (@thebluematt suggested osinterface).

Some unused functions have been completely removed, some functions of limited use that are only used in one place have been moved where they are used (see commits).

Breaks dependency of sha256.cpp on all the things pulled in by util. This makes many include-time dependencies explicit, which before were ‘inherited’ from include files.

laanwj added the label Improvement on Aug 21, 2014

theuni commented at 7:03 pm on August 21, 2014: member

Looks great, I’m verifying the c/p now. Several dependencies on boost could be dropped, I assume you’re leaving that for a follow-up?

theuni commented at 7:38 pm on August 21, 2014: member

Verfied the code movement with c/p, all is clean with 2 exceptions. I’m only mentioning these for the sake of being thorough, I assume they’re harmless.

GetTimeMicros and MilliSleep have moved from inline->non-inline. I have no clue if they actually ended up being inlined for real when compiled before, but if so, that may mean a functional change here due to the timing/precision involved.

That aside, ACK.

laanwj commented at 6:05 am on August 22, 2014: member

If you see any further dependencies on boost - especially includes in headers - that can be removed let me know. This has the overall effect of including less boost headers per compilation unit which means faster compile times.

And indeed: GetTimeMillis/Micros are no longer inline because they pull in a compile-time dependency on boost::posix_time. Having them implemented in utiltime.cpp means that the boost usage is at least isolated. It could theoretically affect timings - but what’s one extra function call in microseconds, who knows what boost::posixtime::* is doing for awful stuff internally. To reliably benchmark small times one should use CPU performance counters (and yes, inlined :-). But IIRC we only benchmark larger time-spans.

laanwj force-pushed on Aug 22, 2014

laanwj commented at 7:43 am on August 22, 2014: member

PASSED! Hah, take that @BitcoinPullTester (for some reason the pulltester environment requires some more explicit #includes of boost/thread.hpp, so had to add those one by one, sorry for the spam)

theuni commented at 3:58 pm on August 22, 2014: member

@laanwj re: inlines, sounds good.

As for the boost deps, I’m not sure if there are others, but I noticed this one right away in utilstring.cpp

0bool IsHex(const string& str)
1{
2 BOOST_FOREACH(char c, str)
3 {
4 if (HexDigit(c) < 0)

->

0bool IsHex(const string& str)
1{
2 for(std::string::const_iterator c(str.begin()); c != str.end(); ++c)
3 {
4 if (HexDigit(*c) < 0)

That would mean dropping boost completely for that object.

laanwj commented at 6:37 am on August 23, 2014: member

@theuni I thought about that, although I like keeping FOREACH, so that they can be easily replaced when the time for c++11 comes. boost/foreach.hpp is header-only.

TheBlueMatt commented at 10:34 pm on August 23, 2014: member

I havent verified the c/p yet (will do that after squash before merge), but you’ve definitely got my conceptual ACK. I might consider changing utilmoney to uilmoneystr or just moneystr, utilstring to utilstrencodings or something and splitting the new util up further into something like arguments/env and logging (but that may be better in a separate pull because some of it isnt clear).

in src/rpcprotocol.cpp: in ea0f29e9ee outdated

 5@@ -6,6 +6,11 @@
 6 #include "rpcprotocol.h"
 7 
 8 #include "util.h"
 9+#include "version.h"
10+#include "version.h"

sipa commented at 11:12 pm on August 24, 2014:

2x version.h?

sipa commented at 11:28 pm on August 24, 2014: member

Untested ACK; I didn’t verify it to be move-only.

laanwj commented at 7:41 am on August 25, 2014: member

@TheBlueMatt I’m not going to do any further splitting in this pull. Those renames sound fine to me though.

laanwj force-pushed on Aug 25, 2014

TheBlueMatt commented at 10:08 am on August 25, 2014: member

ut (but verified cp) ACK

jgarzik commented at 7:38 pm on August 25, 2014: contributor

ut ACK

“utilstrencodings” is a lame name. utilstrcode or strutil ?

sipa commented at 8:25 pm on August 25, 2014: member

Untested ACK, but verified move-only.

I will refrain from joining the filename bikeshedding.

laanwj commented at 6:51 am on August 26, 2014: member

@jgarzik It was just utilstring, then I renamed it after @TheBlueMatt complained about the name once already. I won’t keep renaming them. Go fight it out with him first..

TheBlueMatt commented at 7:30 am on August 26, 2014: member

@jgarzik now its bikeshedding, I dont care either way, it was just a suggestion.

Move CMedianFilter to timedata.cpp

Now that we no longer use the median filter to keep track of
the number of blocks of peers, that's the only place it is used.

d1e26d4e71

Remove unused `alignup` function from util.h 121d6ad9db

Remove unused function `ByteReverse` from util.h f780e65ac6

Move SetThreadPriority implementation to util.cpp instead of the header

Put the THREAD_* and PRIO_ constants in compat.h.

610a8c0759

move functions in main and net to implementation files 651480c8e4

Move functions in wallet.h to implementation file

Breaks compile-time dependency of wallet.h on util.

af8297c010

Move `S_I*` constants and `MSG_NOSIGNAL` to compat.h b4aa769bcb

Move `*Version()` functions to version.h/cpp 6e5fd003e0

Move `COIN` and `CENT` to core.h

Eventually these should end up in `money.h` after monetary
amounts are typedef'ed, but at least they don't belong in `util.h`.

f841aa2892

Split up util.cpp/h

Split up util.cpp/h into:

- string utilities (hex, base32, base64): no internal dependencies, no dependency on boost (apart from foreach)
- money utilities (parsesmoney, formatmoney)
- time utilities (gettime*, sleep, format date):
- and the rest (logging, argument parsing, config file parsing)

The latter is basically the environment and OS handling,
and is stripped of all utility functions, so we may want to
rename it to something else than util.cpp/h for clarity (Matt suggested
osinterface).

Breaks dependency of sha256.cpp on all the things pulled in by util.

ad49c256c3

laanwj force-pushed on Aug 26, 2014

BitcoinPullTester commented at 11:39 am on August 26, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4748_ad49c256c33bfe4088fd3c7ecb7d28cb81a8fc70/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

sipa commented at 2:51 pm on August 26, 2014: member

Merging, as this will likely be outdated soon otherwise.

File renames can easily be done later if necessary.

sipa merged this on Aug 26, 2014

sipa closed this on Aug 26, 2014

sipa referenced this in commit 3da58b216b on Aug 26, 2014

laanwj referenced this in commit a2a3f4ba2c on Jun 27, 2016

laanwj referenced this in commit 1666e37fb7 on Jun 27, 2016

laanwj referenced this in commit 695041e495 on Jun 27, 2016

zkbot referenced this in commit 6bad499c2a on Oct 25, 2016

protonn referenced this in commit 2e09c3d7f1 on May 7, 2017

lateminer referenced this in commit 0320890116 on Jan 3, 2019

MarcoFalke locked this on Sep 8, 2021

Split up util.cpp/h #4748