Refactor: Remove using namespace <xxx> from bench/ & test/ sources #9281

kallewoof · 2016-12-05T09:13:29Z

This is a sub-set of #9235, including only the bench and test dirs.

Same-binaries check:

$ ../bitcoin-maintainer-tools/build-for-compare.py 53442af0aac3838179fad79a65512ee8c5603922 73f41190b91dce9c125b1828b18f7625e0200145 --executables "src/bitcoind,src/bitcoin-cli,src/bitcoin-tx"
[...]
make[1]: Leaving directory '/tmp/repo/src'
>>> [do_build] Copying object files...
>>> [do_build] Performing basic analysis pass...
>>> [do_build] Use these commands to compare results:
>>> [do_build] $ sha256sum /tmp/compare/*.stripped
>>> [do_build] $ git diff -W --word-diff /tmp/compare/53442af0aac3838179fad79a65512ee8c5603922 /tmp/compare/73f41190b91dce9c125b1828b18f7625e0200145
$ sha256sum /tmp/compare/*.stripped
2827166eaec5c3f5517e0000f3afcecbd00911388a0ae4453297083050dd468b  /tmp/compare/bitcoin-cli.53442af0aac3838179fad79a65512ee8c5603922.stripped
2827166eaec5c3f5517e0000f3afcecbd00911388a0ae4453297083050dd468b  /tmp/compare/bitcoin-cli.73f41190b91dce9c125b1828b18f7625e0200145.stripped
95fb9373c45c45b3d2882f2895b5f5c824f0b5b1d24935f379eafc59abd115e2  /tmp/compare/bitcoind.53442af0aac3838179fad79a65512ee8c5603922.stripped
95fb9373c45c45b3d2882f2895b5f5c824f0b5b1d24935f379eafc59abd115e2  /tmp/compare/bitcoind.73f41190b91dce9c125b1828b18f7625e0200145.stripped
aabf6634d1e0c177dc9db924b8b3ad8d231cfff693b4adb97be6d66d536dce85  /tmp/compare/bitcoin-tx.53442af0aac3838179fad79a65512ee8c5603922.stripped
aabf6634d1e0c177dc9db924b8b3ad8d231cfff693b4adb97be6d66d536dce85  /tmp/compare/bitcoin-tx.73f41190b91dce9c125b1828b18f7625e0200145.stripped
$ git diff -W --word-diff /tmp/compare/53442af0aac3838179fad79a65512ee8c5603922 /tmp/compare/73f41190b91dce9c125b1828b18f7625e0200145
$

For bench/bench_bitcoin:

$ sha256sum /tmp/compare/*.stripped
5e60275a2f4972a0d6e1d727f055267cc7bed2c35beab0d27d9aff8d4327e684  /tmp/compare/bench_bitcoin.53442af0aac3838179fad79a65512ee8c5603922.stripped
5e60275a2f4972a0d6e1d727f055267cc7bed2c35beab0d27d9aff8d4327e684  /tmp/compare/bench_bitcoin.73f41190b91dce9c125b1828b18f7625e0200145.stripped

The affected files and namespaces:

bench/bench.cpp:          benchmark
bench/coin_selection.cpp: std
test/addrman_tests.cpp:   std
test/bloom_tests.cpp:     std
test/dbwrapper_tests.cpp: std
test/hash_tests.cpp:      std [unused]
test/key_tests.cpp:       std
test/multisig_tests.cpp:  std
test/net_tests.cpp:       std
test/netbase_tests.cpp:   std
test/pmt_tests.cpp:       std [unused]
test/pow_tests.cpp:       std [unused]
test/rpc_tests.cpp:       std
test/script_P2SH_tests.cpp: std
test/script_tests.cpp:    std
test/serialize_tests.cpp: std
test/sigopcount_tests.cpp: std
test/streams_tests.cpp:   std [using boost::assign remains, for +=() operator]
test/timedata_tests.cpp:  std [unused]
test/transaction_tests.cpp: std
test/univalue_tests.cpp:  std
test/util_tests.cpp:      std [unused]

paveljanik · 2016-12-05T12:36:39Z

src/test/streams_tests.cpp

@@ -9,8 +9,7 @@
 #include <boost/assign/std/vector.hpp> // for 'operator+=()'
 #include <boost/assert.hpp>
 #include <boost/test/unit_test.hpp>
-
-using namespace std;
+
 using namespace boost::assign; // bring 'operator+=()' into scope


Forgetten 'using namespace'?

You mean boost::assign? It's not trivial to remove, as it actually adds an operator so that things like the key statement below are possible.

std::vector<unsigned char> key; [...] key += '\x00','\x00';

IIRC in C++11, you can initialise this vector with

key = {'\x00','\x00'};

But this is probably outside of the scope of this PR.

Yeah, but you can't add onto it afterwards (using +=), at least AFAIK.
I think a nice compromise is to move the using namespace boost::assign into the functions using it. I can do that if people prefer.

Yup, but the code is always using it after .clear()...

Wow, yeah you're right. I didn't realize that.

Actually, I don't think I'll fix it (as you said initially, it's outside the scope of this PR) as it would no doubt break the same-binaries condition I've been working hard to retain. With a big update I think upholding that condition is worthwhile.

Agree, not for this PR as I said.

It is also possible to scope the using boost::* to the only function where it is used. But better keep it as is, when there are plans to move to cpp11 anyway.

maflcko · 2016-12-31T20:24:04Z

Needs rebase.

maflcko · 2017-01-01T00:00:04Z

src/test/dbwrapper_tests.cpp

-using namespace boost::filesystem;
-
+
+namespace io = boost::filesystem;


Might be better to just keep using namespace boost::filesystem or specify the full namespace instead.

Aliasing a namespace is quite different from using it, I think. I prefer removing the alias and just using the full namespace instead, in that case.

Thx, fine with me.

…ce files.

kallewoof · 2017-01-02T12:03:43Z

Rebased. Will do a same-binary check in a couple of days if needed (wrong machine atm).

maflcko · 2017-01-03T11:21:12Z

utACK 73f4119

maflcko · 2017-01-03T11:31:53Z

Will do a same-binary check in a couple of days if needed

Ping me when you are done, so I don't forget about this pull.

kallewoof · 2017-01-04T02:41:59Z

@MarcoFalke Same-binary check done and indicates bitcoin-cli, bitcoin-tx, bitcoind are all identical. (See updated OP.)

maflcko · 2017-01-04T13:30:35Z

@kallewoof Can you run it for the affected binaries instead? 😛

      test/test_bitcoin
      bench/bench_bitcoin

kallewoof · 2017-01-05T02:34:03Z

@MarcoFalke The binary compare util explicitly disables tests. I removed that and tried and I got differences. I believe this is because the tests use macros which capture the line numbers, which obviously change just by adding or removing lines, even if the execution itself doesn't change.

bench does pass the binary check though:

$ sha256sum /tmp/compare/*.stripped
5e60275a2f4972a0d6e1d727f055267cc7bed2c35beab0d27d9aff8d4327e684  /tmp/compare/bench_bitcoin.53442af0aac3838179fad79a65512ee8c5603922.stripped
5e60275a2f4972a0d6e1d727f055267cc7bed2c35beab0d27d9aff8d4327e684  /tmp/compare/bench_bitcoin.73f41190b91dce9c125b1828b18f7625e0200145.stripped
f6dd571bb46b1e9a472b9cfcd5087b0fed6f47974e91a9ec4004581752a985ae  /tmp/compare/test_bitcoin.53442af0aac3838179fad79a65512ee8c5603922.stripped
79a56fa4bb4dfbd5ff75ea3d5e5178e078f37640d9e53c3b8aee210c112e2e48  /tmp/compare/test_bitcoin.73f41190b91dce9c125b1828b18f7625e0200145.stripped

Edit: another likely reason for a mismatch is that test macros tend to retain the line that failed as a string (e.g. #define FOO(x) if (!x) print("failure: %s", #x)), which would mean binary mismatch for vector<string> -> std::vector<std::string>. I think bin check for src/test is probably impossible. Even by reinserting empty lines in place of the removed using namespace <xxx>;, the binary check still fails.

…t/ sources 73f4119 Refactoring: Removed using namespace <xxx> from bench/ and test/ source files. (Karl-Johan Alm)

…/ & test/ sources 73f4119 Refactoring: Removed using namespace <xxx> from bench/ and test/ source files. (Karl-Johan Alm)

Micro-benchmarking framework part 1 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6733 - bitcoin/bitcoin#6770 - bitcoin/bitcoin#6892 - Excluding changes to `src/policy/policy.h` which we don't have yet. - bitcoin/bitcoin#7934 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#8039 - bitcoin/bitcoin#8107 - bitcoin/bitcoin#8115 - bitcoin/bitcoin#8914 - Required resolving several merge conflicts in code that had been refactored upstream. The changes were simple enough that I decided it was okay to impose merge conflicts on pulling in those refactors later. - bitcoin/bitcoin#9200 - bitcoin/bitcoin#9202 - Adds support for measuring CPU cycles, which is later removed in an upstream PR after the refactor. I am including it to reduce future merge conflicts. - bitcoin/bitcoin#9281 - Only changes to `src/bench/bench.cpp` - bitcoin/bitcoin#9498 - bitcoin/bitcoin#9712 - bitcoin/bitcoin#9547 - bitcoin/bitcoin#9505 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#9792 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#10272 - bitcoin/bitcoin#10395 - Only changes to `src/bench/` - bitcoin/bitcoin#10735 - Only changes to `src/bench/base58.cpp` - bitcoin/bitcoin#10963 - bitcoin/bitcoin#11303 - Only the benchmark backend change. - bitcoin/bitcoin#11562 - bitcoin/bitcoin#11646 - bitcoin/bitcoin#11654 This pulls in all changes to the micro-benchmark framework prior to December 2017, when it was rewritten. The rewrite depends on other upstream PRs we have not pulled in yet. This does not pull in all benchmarks prior to December 2017. It leaves out benchmarks that either test code we do not have yet (except for the `FastRandomContext` refactor, which I decided to pull in), or would require rewrites to work with our changes to the codebase.

82f9088 Refactor: Remove using namespace <xxx> from /dbwrapper_tests (random-zebra) 6db0b37 rpc: make `gettxoutsettinfo` run lock-free (random-zebra) f009cf4 Do not shadow members in dbwrapper (random-zebra) b7e540c dbwrapper: Move `HandleError` to `dbwrapper_private` (random-zebra) 43004d0 leveldbwrapper file rename to dbwrapper.* (random-zebra) c882dd9 leveldbwrapper symbol rename: Remove "Level" from class, etc. names (random-zebra) f6496da leveldbwrapper: Remove unused .Prev(), .SeekToLast() methods (random-zebra) cacf3c2 Fix chainstate serialized_size computation (random-zebra) a2a3d33 Add tests for CLevelDBBatch, CLevelDBIterator (random-zebra) 94150ac Encapsulate CLevelDB iterators cleanly (random-zebra) 21df7cc [DB] Refactor leveldbwrapper (random-zebra) 2251db3 change hardcoded character constants to a set of descriptive named co… (random-zebra) Pull request description: This backports a series of updates and cleanups to the LevelDB wrapper from: - bitcoin#5707 - bitcoin#6650 [`*`] - bitcoin#6777 [`*`] - bitcoin#6865 - bitcoin#6873 - bitcoin#7927 [`*`] - bitcoin#8467 - bitcoin#6290 - bitcoin#9281 PIVX-specific edits were required to keep the sporks and zerocoin databases in line. [`*`] NOTE: excluding the obfuscation of databases by xoring data, as we might not want this feature (e.g. as zcash/zcash#2598). Otherwise it can be discussed, and added, with a separate PR. ACKs for top commit: furszy: Re ACK 82f9088 . Fuzzbawls: ACK 82f9088 Tree-SHA512: 1e4a75621d2ec2eb68e01523d15321d1d2176b81aac0525617852899ab38c9b4980daecb9056d054e7961fc758a22143edf914c40d1819144a394f2869a8ad57

3f3edde [Bench] Use PIVX address in Base58Decode test (random-zebra) 5a1be90 [Travis] Disable benchmark framework for trusty test (random-zebra) 1bd89ac Initialize recently introduced non-static class member lastCycles to zero in constructor (random-zebra) ec60671 Require a steady clock for bench with at least micro precision (random-zebra) 84069ce bench: prefer a steady clock if the resolution is no worse (random-zebra) 38367b1 bench: switch to std::chrono for time measurements (random-zebra) a24633a Remove countMaskInv caching in bench framework (random-zebra) 9e9bc22 Restore default format state of cout after printing with std::fixed/setprecision (random-zebra) 3dd559d Avoid static analyzer warnings regarding uninitialized arguments (random-zebra) e85f224 Replace boost::function with std::function (C++11) (random-zebra) 98c0857 Prevent warning: variable 'x' is uninitialized (random-zebra) 7f0d4b3 FastRandom benchmark (random-zebra) d9fa0c6 Add prevector destructor benchmark (random-zebra) e1527ba Assert that what might look like a possible division by zero is actually unreachable (random-zebra) e94cf15 bench: Fix initialization order in registration (random-zebra) 151c25f Basic CCheckQueue Benchmarks (random-zebra) 51aedbc Use std:thread:hardware_concurrency, instead of Boost, to determine available cores (random-zebra) d447613 Use real number of cores for default -par, ignore virtual cores (random-zebra) 9162a56 [Refactoring] Removed using namespace <xxx> from bench/ sources (random-zebra) 5c07f67 bench: Add support for measuring CPU cycles (random-zebra) 41ce1ed bench: Fix subtle counting issue when rescaling iteration count (random-zebra) 68ea794 Avoid integer division in the benchmark inner-most loop. (random-zebra) 3fa4f27 bench: Added base58 encoding/decoding benchmarks (random-zebra) 4442118 bench: Add crypto hash benchmarks (random-zebra) a5179b6 [Trivial] ensure minimal header conventions (random-zebra) 8607d6b Support very-fast-running benchmarks (random-zebra) 4aebb60 Simple benchmarking framework (random-zebra) Pull request description: Introduces the benchmarking framework, loosely based on google's micro-benchmarking library (https://github.com/google/benchmark), ported from Bitcoin, up to 0.16. The benchmark framework is hard-coded to run each benchmark for one wall-clock second, and then spits out .csv-format timing information to stdout. Backported PR: - bitcoin#6733 - bitcoin#6770 - bitcoin#6892 - bitcoin#8039 - bitcoin#8107 - bitcoin#8115 - bitcoin#9200 - bitcoin#9202 - bitcoin#9281 - bitcoin#6361 - bitcoin#10271 - bitcoin#9498 - bitcoin#9712 - bitcoin#9547 - bitcoin#9505 (benchmark only. Rest was in #1557) - bitcoin#9792 (benchmark only. Rest was in #643) - bitcoin#10272 - bitcoin#10395 (base58 only) - bitcoin#10963 - bitcoin#11303 (first commit) - bitcoin#11562 - bitcoin#11646 - bitcoin#11654 Current output of `src/bench/bench_pivx`: ``` #Benchmark,count,min(ns),max(ns),average(ns),min_cycles,max_cycles,average_cycles Base58CheckEncode,131072,7697,8065,7785,20015,20971,20242 Base58Decode,294912,3305,3537,3454,8595,9198,8981 Base58Encode,180224,5498,6020,5767,14297,15652,14994 CCheckQueueSpeed,320,3159960,3535173,3352787,8216030,9191602,8717388 CCheckQueueSpeedPrevectorJob,96,9184484,11410840,10823070,23880046,29668680,28140445 FastRandom_1bit,320,3143690,4838162,3199156,8173726,12579373,8317941 FastRandom_32bit,60,17097612,17923669,17367440,44454504,46602306,45156079 PrevectorClear,3072,334741,366618,346731,870340,953224,901516 PrevectorDestructor,2816,344233,368912,357281,895022,959187,928948 RIPEMD160,288,3404503,3693917,3577774,8851850,9604334,9302363 SHA1,384,2718128,2891558,2802513,7067238,7518184,7286652 SHA256,176,6133760,6580005,6239866,15948035,17108376,16223916 SHA512,240,4251468,4358706,4313463,11054006,11332826,11215186 Sleep100ms,10,100221470,100302411,100239073,260580075,260790726,260625870 ``` NOTE: Not all the tests have been pulled yet (as we might not have the code being tested, or it would require rewrites to work with our different code base), but the framework is updated to December 2017. ACKs for top commit: Fuzzbawls: ACK 3f3edde Tree-SHA512: c283311a9accf6d2feeb93b185afa08589ebef3f18b6e86980dbc3647b9845f75ac9ecce2f1b08738d25ceac36596a2c89d41e4dbf3b463502aa695611aa1f8e

fanquake added the Refactoring label Dec 5, 2016

maflcko added the Tests label Dec 5, 2016

paveljanik reviewed Dec 5, 2016

View reviewed changes

maflcko reviewed Jan 1, 2017

View reviewed changes

Refactoring: Removed using namespace <xxx> from bench/ and test/ sour…

73f4119

…ce files.

kallewoof force-pushed the no-using-namespace-bench-test branch from bb43489 to 73f4119 Compare January 2, 2017 12:02

kallewoof mentioned this pull request Jan 4, 2017

[WIP] Refactor: Remove all uses of using namespace in all source files. #9235

Closed

kallewoof changed the title ~~Refactor: Removed using namespace <xxx> from bench/ & test/ sources~~ Refactor: Remove using namespace <xxx> from bench/ & test/ sources Jan 4, 2017

maflcko merged commit 73f4119 into bitcoin:master Jan 5, 2017

maflcko pushed a commit that referenced this pull request Jan 5, 2017

Merge #9281: Refactor: Remove using namespace <xxx> from bench/ & tes…

4cfd57d

…t/ sources 73f4119 Refactoring: Removed using namespace <xxx> from bench/ and test/ source files. (Karl-Johan Alm)

kallewoof deleted the no-using-namespace-bench-test branch January 5, 2017 10:34

str4d mentioned this pull request Feb 22, 2019

Micro-benchmarking framework part 1 zcash/zcash#3858

Merged

random-zebra mentioned this pull request May 23, 2020

[DB] Bitcoin 0.12-0.14 dbwrapper improvements PIVX-Project/PIVX#1629

Merged

random-zebra mentioned this pull request May 27, 2020

[Tests] Benchmarking Framework PIVX-Project/PIVX#1650

Merged

lyricidal mentioned this pull request Aug 27, 2021

[Upstream] [DB] Bitcoin 0.12-0.14 dbwrapper improvements PRCYCoin/PRCYCoin#102

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor: Remove using namespace <xxx> from bench/ & test/ sources #9281

Refactor: Remove using namespace <xxx> from bench/ & test/ sources #9281

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		using namespace boost::filesystem;


		namespace io = boost::filesystem;

Refactor: Remove using namespace <xxx> from bench/ & test/ sources #9281

Refactor: Remove using namespace <xxx> from bench/ & test/ sources #9281

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!