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

kallewoof commented at 9:13 AM on December 5, 2016: member

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]

fanquake added the label Refactoring on Dec 5, 2016

MarcoFalke added the label Tests on Dec 5, 2016

in src/test/streams_tests.cpp:None in bb43489e18 outdated

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

paveljanik commented at 12:36 PM on December 5, 2016:

Forgetten 'using namespace'?

kallewoof commented at 1:14 PM on December 5, 2016:

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';

paveljanik commented at 2:22 PM on December 5, 2016:

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

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

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

kallewoof commented at 2:27 PM on December 5, 2016:

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.

paveljanik commented at 2:29 PM on December 5, 2016:

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

kallewoof commented at 2:32 PM on December 5, 2016:

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

kallewoof commented at 12:40 AM on December 6, 2016:

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.

paveljanik commented at 6:51 AM on December 6, 2016:

Agree, not for this PR as I said.

MarcoFalke commented at 8:51 PM on December 31, 2016:

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.

MarcoFalke commented at 8:24 PM on December 31, 2016: member

Needs rebase.

Refactoring: Removed using namespace <xxx> from bench/ and test/ source files. 73f41190b9

in src/test/dbwrapper_tests.cpp:None in bb43489e18 outdated

  14 | -using namespace std;
  15 | -using namespace boost::assign; // bring 'operator+=()' into scope
  16 | -using namespace boost::filesystem;
  17 | -         
  18 | +
  19 | +namespace io = boost::filesystem;

MarcoFalke commented at 12:00 AM on January 1, 2017:

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

kallewoof commented at 11:25 AM on January 2, 2017:

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.

MarcoFalke commented at 11:28 AM on January 2, 2017:

Thx, fine with me.

kallewoof force-pushed on Jan 2, 2017

kallewoof commented at 12:03 PM on January 2, 2017: member

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

MarcoFalke commented at 11:21 AM on January 3, 2017: member

utACK 73f41190b91dce9c125b1828b18f7625e0200145

MarcoFalke commented at 11:31 AM on January 3, 2017: member

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 commented at 2:41 AM on January 4, 2017: member

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

kallewoof renamed this:
~~Refactor: Removed using namespace <xxx> from bench/ & test/ sources~~
Refactor: Remove using namespace <xxx> from bench/ & test/ sources
on Jan 4, 2017

MarcoFalke commented at 1:30 PM on January 4, 2017: member

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

      test/test_bitcoin
      bench/bench_bitcoin

kallewoof commented at 2:34 AM on January 5, 2017: member

@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.

MarcoFalke merged this on Jan 5, 2017

MarcoFalke closed this on Jan 5, 2017

MarcoFalke referenced this in commit 4cfd57d2e3 on Jan 5, 2017

kallewoof deleted the branch on Jan 5, 2017

codablock referenced this in commit 8c4c6063a4 on Jan 18, 2018

andvgal referenced this in commit ccdd3e52dc on Jan 6, 2019

CryptoCentric referenced this in commit 51631f1da4 on Feb 26, 2019

zkbot referenced this in commit aa225ebb0b on Jan 24, 2020

zkbot referenced this in commit 74ff73abab on Jan 24, 2020

random-zebra referenced this in commit 73d26f20e9 on May 27, 2020

furszy referenced this in commit 4ed15cc69d on Jun 8, 2020

MarcoFalke locked this on Sep 8, 2021