Skip to content

Conversation

@kallewoof
Copy link
Contributor

@kallewoof kallewoof commented Nov 29, 2016

According to the "Source code organization" section in Developer notes, using namespace should be avoided. This commit removes all uses of using namespace <xxx> in all source files, including test and bench related files, as well as the univalue library.

This PR is being split into multiple PR's as described below.

bench and test (#9281):

bench/bench.cpp:          benchmark
bench/coin_selection.cpp: std
test/addrman_tests.cpp:   std
test/bloom_tests.cpp:     std
test/dbwrapper_tests.cpp: std [namespace boost::filesystem mapped to io for brevity]
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]

rpc and script (#9476):

rpc/blockchain.cpp:       std
rpc/client.cpp:           std
rpc/mining.cpp:           std
rpc/misc.cpp:             std
rpc/net.cpp:              std
rpc/protocol.cpp:         std
rpc/rawtransaction.cpp:   std
rpc/server.cpp:           RPCServer [unused], std
script/interpreter.cpp:   std
script/ismine.cpp:        std
script/script.cpp:        std
script/sign.cpp:          std
script/standard.cpp:      std

wallet and util* (#9643):

util.cpp:                 std
utilmoneystr.cpp:         std
utilstrencodings.cpp:     std
utiltime.cpp:             std [unused]
wallet/db.cpp:            std
wallet/rpcdump.cpp:       std
wallet/rpcwallet.cpp:     std
wallet/test/wallet_tests.cpp: std
wallet/wallet.cpp:        std
wallet/walletdb.cpp:      std

Remaining (#9644):

bloom.cpp:                std
chain.cpp:                std [unused]
core_read.cpp:            std
core_write.cpp:           std
init.cpp:                 std, boost::filesystem [inside void CleanupBlockRevFiles()]
merkleblock.cpp:          std
miner.cpp:                std
net_processing.cpp:       std
rest.cpp:                 std
timedata.cpp:             std
txdb.cpp:                 std
txmempool.cpp:            std
validation.cpp:           std

@kallewoof kallewoof changed the title Refactor: Removes all uses of using namespace in all source files. Refactor: Removes all uses of using namespace in all source files. Nov 29, 2016
@dcousens
Copy link
Contributor

dcousens commented Nov 29, 2016

concept ACK, the diff is small enough I don't see this is as a change that would ruin history, while still being a large scale [trivial] refactor.

@paveljanik
Copy link
Contributor

Concept ACK. It has to be done.

Do not change univalue tree here. Why is using namespace bench bad here?

@kallewoof
Copy link
Contributor Author

kallewoof commented Nov 29, 2016

@paveljanik I reverted the univalue changes. Using namespaces anywhere is generally a bad idea. The primary reason is explained better than I can here. The gist of it is captured in this sentence: "Library Foo 2.0 could introduce a function, Quux(), that is an unambiguously better match for some of your calls to Quux() than the bar::Quux() your code called for years. Then your code still compiles, but it silently calls the wrong function and does god-knows-what. That's about as bad as things can get." Also, a user may see namespace use in one file and think it's fine to do it elsewhere.

@kallewoof
Copy link
Contributor Author

kallewoof commented Nov 29, 2016

FWIW, binary check still passes (updated initial post with output for e81f1a0).

@jonasschnelli
Copy link
Contributor

Concept ACK

@laanwj
Copy link
Member

laanwj commented Nov 29, 2016

"Using namespaces anywhere is generally a bad idea."

To disambiguate this: The use of namespaces in general is not a bad idea, right? Namespaces can IMO be a useful code-organization principle. But the using namespace statement is - that part I agree on, certainly in global scope.

This changes so many source files though :( Let's make sure that we first merge in the features and fixes that we really want in 0.14, to avoid giving people extra rebasing busywork.

@kallewoof
Copy link
Contributor Author

@laanwj Oh, yes apologies about ambiguous wording. If I were to decide there'd be a lot more namespaces in the code, but one step at a time, right? :)

There are a lot of files changed, yeah. Luckily, rebasing is extremely trivial and straight-forward, and that goes both ways, so I will gladly rebase this PR once you feel ready for it.

A secondary option is that you pick files that you want and I make separate PR:s for those files and we do this iteratively.

@fanquake
Copy link
Member

fanquake commented Dec 2, 2016

@kallewoof I've merged this, and can still see lots of usage of namespace std i.e in addrman_tests, or dbwrapper_tests. Have they been left on purpose?

@kallewoof
Copy link
Contributor Author

@fanquake I used a "find all" for using namespace and fixed everything, but you're right, somehow there's a lot left.

For starters I'm going to fix the leftovers so this PR is complete. Thanks for pointing that out!

@kallewoof kallewoof changed the title Refactor: Removes all uses of using namespace in all source files. [WIP] Refactor: Removes all uses of using namespace in all source files. Dec 2, 2016
@kallewoof kallewoof force-pushed the no-using-ns2 branch 3 times, most recently from 9d76b00 to a2c6066 Compare December 5, 2016 06:11
@kallewoof kallewoof changed the title [WIP] Refactor: Removes all uses of using namespace in all source files. [WIP] Refactor: Remove all uses of using namespace in all source files. Jan 4, 2017
@jtimon
Copy link
Contributor

jtimon commented Jan 13, 2017

Concept ACK, but this may be too disruptive for a single PR, perhaps you should consider dividing it in a few smaller ones. Maybe start with namespace std?
Personally other uses like using namespace benchmark ins src/bench/bench.cpp don't bother me that much, but I'm not against removing them either.
Another advantage of dividing it in smaller ones (apart from each being more reviewable and mergeable separated) is that they will individually require rebase less often.
Needs rebase.

@kallewoof
Copy link
Contributor Author

Yeah, I started doing that (2 PRs so far; I plan to make the next one whenever the current one is merged to not swamp the members), but felt I could keep this PR around as a central point of sorts. I am closing this one though as it can be closed and still be referenced.

@kallewoof kallewoof closed this Jan 13, 2017
@kallewoof kallewoof deleted the no-using-ns2 branch October 17, 2019 09:02
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants