Remove global symbols: Avoid using the global namespace if possible by practicalswift · Pull Request #15622 · bitcoin/bitcoin

practicalswift · 2019-03-19T08:43:25Z

Remove global symbols: Avoid using the global namespace if possible.

Partially resolves #15612 ("Reduce the number of global symbols used").

Change in global symbols as reported by nm bitcoind before vs after:

$ diff -u <(nm src/bitcoind-before | c++filt | grep -E '^[0-9a-f]+ [A-Z] ' | cut -f3- -d' ' | sort -u) \
          <(nm src/bitcoind-after  | c++filt | grep -E '^[0-9a-f]+ [A-Z] ' | cut -f3- -d' ' | sort -u) \
          | grep -E '^[+-][^+-]'
-boundSockets
-cs_warnings
-eventHTTP
-fFeeEstimatesInitialized
-fLargeWorkForkFound
-fLargeWorkInvalidChainFound
-pathHandlers
-strMiscWarning[abi:cxx11]
-threadHTTP

src/httpserver.cpp

promag · 2019-03-20T18:22:15Z

Concept ACK, change looks good.

maflcko · 2019-03-20T21:21:55Z

utACK b112097f8e5f96e96db47e2eef9870d6f0431677. Also checked that the binary is 9kB smaller.

Empact · 2019-03-21T08:14:22Z

utACK b112097

src/init.cpp

practicalswift · 2019-03-22T09:39:56Z

Now removing redundant static:s in unnamed namespaces as requested by @promag and @Empact.

Please re-review :-)

promag · 2019-03-22T14:50:31Z

utACK 9638da7.

lucayepa · 2019-03-23T23:52:12Z

I think we can add some language on this policy in doc/developer-notes.md. It will help new developers. I agree on a zero-globals policy, enforced with singletons, or with a "state" class that is a container of all the globals, or with a dedicated namespace. This way, every time a new global has to be added (rarely), we can see a clear change in a single place. In my mind this policy should not be enforced from now, but should be accepted, then written in doc/developer-notes.md or elsewhere, and enforced after a couple of years, when all the open PRs are already compliant.

I've just described this method of long grace periods in my comment on #15465.

practicalswift · 2019-03-27T20:28:57Z

@lucayepa I would like to restrict the scope of this PR to these specific code changes which have already been (ut)ACK:ed.

Empact · 2019-04-03T05:28:02Z

utACK 9638da7

src/init.cpp

sipa · 2019-04-16T20:35:18Z

It's a bit confusing to call this"Remove globals"; a global inside a namespace is still a global.

practicalswift · 2019-04-16T20:46:05Z

@sipa I've now clarified that this refers to global (external) symbols :-)

practicalswift · 2019-05-07T16:59:50Z

utACK:s from @Empact and @promag, one stale utACK from @MarcoFalke and zero NACK:s

@MarcoFalke, are we ready to move forward with this one to get a slightly less polluted global namespace? :-)

ajtowns · 2019-05-22T06:36:20Z

I would have thought it would be better to write these as static ... than namespace { \n ... \n } -- you can grep for the former relatively easily, but not really the latter, and it seems more consistent with the current style... It gives a +9-9 diff which seems better than +17-9...

Either way, I don't see the point of dropping static just because it's redundant; that seems like needless churn. After stripping the binary, i don't see any size difference in bitcoind amongst these variants on Linux with clang fwiw.

practicalswift · 2019-05-24T13:21:11Z

@ajtowns I've now reverted to the original version of the PR which is in accordance with your suggestions. Please re-review :-)

@promag @MarcoFalke @Empact Would you mind re-reviewing? :-)

src/warnings.cpp

DrahtBot · 2019-05-24T18:14:00Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16084 (scripted-diff: Complete the move from CCriticalSection to identical RecursiveMutex (both are AnnotatedMixinstd::recursive_mutex) by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2019-05-25T14:36:50Z

Please squash the second commit, no need to have a separate commit for a single line stylistic fixup

Rename CCriticalSection to RecursiveMutex (both are AnnotatedMixin<std::recursive_mutex>) ``` $ git grep -E '(typedef|using).*(CCriticalSection|RecursiveMutex)' src/sync.h:using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>; src/sync.h:typedef AnnotatedMixin<std::recursive_mutex> CCriticalSection; ```

practicalswift · 2019-05-25T21:24:13Z

@MarcoFalke Done! Please re-review :-)

… if possible fb43415 Remove global symbols: Avoid using the global namespace if possible (practicalswift) Pull request description: Remove global symbols: Avoid using the global namespace if possible. Partially resolves #15612 ("Reduce the number of global symbols used"). Change in global symbols as reported by `nm bitcoind` before vs after: ``` $ diff -u <(nm src/bitcoind-before | c++filt | grep -E '^[0-9a-f]+ [A-Z] ' | cut -f3- -d' ' | sort -u) \ <(nm src/bitcoind-after | c++filt | grep -E '^[0-9a-f]+ [A-Z] ' | cut -f3- -d' ' | sort -u) \ | grep -E '^[+-][^+-]' -boundSockets -cs_warnings -eventHTTP -fFeeEstimatesInitialized -fLargeWorkForkFound -fLargeWorkInvalidChainFound -pathHandlers -strMiscWarning[abi:cxx11] -threadHTTP ``` ACKs for commit fb4341: Tree-SHA512: d2f78f6188a992b0e0de8d107e2c494cfa0faa2de4fda634a1d3606d6515633bec86289cf2a2e78ffe467b17b795e2243cc459fb44e0dfe2fc69899506ff61c9

… if possible Summary: fb434159d1 Remove global symbols: Avoid using the global namespace if possible (practicalswift) Pull request description: Remove global symbols: Avoid using the global namespace if possible. Partially resolves #15612 ("Reduce the number of global symbols used"). Change in global symbols as reported by `nm bitcoind` before vs after: ``` $ diff -u <(nm src/bitcoind-before | c++filt | grep -E '^[0-9a-f]+ [A-Z] ' | cut -f3- -d' ' | sort -u) \ <(nm src/bitcoind-after | c++filt | grep -E '^[0-9a-f]+ [A-Z] ' | cut -f3- -d' ' | sort -u) \ | grep -E '^[+-][^+-]' -boundSockets -cs_warnings -eventHTTP -fFeeEstimatesInitialized -fLargeWorkForkFound -fLargeWorkInvalidChainFound -pathHandlers -strMiscWarning[abi:cxx11] -threadHTTP ``` ACKs for commit fb4341: Tree-SHA512: d2f78f6188a992b0e0de8d107e2c494cfa0faa2de4fda634a1d3606d6515633bec86289cf2a2e78ffe467b17b795e2243cc459fb44e0dfe2fc69899506ff61c9 Backport of Core [[bitcoin/bitcoin#15622 | PR15622]] Test Plan: While the instructions in the summary do not appear to work, manually inspecting like: ``` diff -u <(nm src/bitcoind-before) <(nm src/bitcoind-after) | less ``` And searching for the above instances indicates they were removed as expected. `ninja check check-functional` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6516

…mespace if possible fb43415 Remove global symbols: Avoid using the global namespace if possible (practicalswift) Pull request description: Remove global symbols: Avoid using the global namespace if possible. Partially resolves bitcoin#15612 ("Reduce the number of global symbols used"). Change in global symbols as reported by `nm bitcoind` before vs after: ``` $ diff -u <(nm src/bitcoind-before | c++filt | grep -E '^[0-9a-f]+ [A-Z] ' | cut -f3- -d' ' | sort -u) \ <(nm src/bitcoind-after | c++filt | grep -E '^[0-9a-f]+ [A-Z] ' | cut -f3- -d' ' | sort -u) \ | grep -E '^[+-][^+-]' -boundSockets -cs_warnings -eventHTTP -fFeeEstimatesInitialized -fLargeWorkForkFound -fLargeWorkInvalidChainFound -pathHandlers -strMiscWarning[abi:cxx11] -threadHTTP ``` ACKs for commit fb4341: Tree-SHA512: d2f78f6188a992b0e0de8d107e2c494cfa0faa2de4fda634a1d3606d6515633bec86289cf2a2e78ffe467b17b795e2243cc459fb44e0dfe2fc69899506ff61c9

fanquake added the Refactoring label Mar 19, 2019

maflcko reviewed Mar 19, 2019

View reviewed changes

src/httpserver.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the globals-1 branch from 596626c to b112097 Compare March 19, 2019 14:19

promag reviewed Mar 21, 2019

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

maflcko reviewed Apr 16, 2019

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

practicalswift changed the title ~~Remove globals: Avoid using the global namespace if possible~~ Remove global symbols: Avoid using the global namespace if possible Apr 16, 2019

practicalswift force-pushed the globals-1 branch from 9638da7 to ec7ee88 Compare May 24, 2019 13:19

maflcko reviewed May 24, 2019

View reviewed changes

src/warnings.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the globals-1 branch from 89fc924 to 43b8d89 Compare May 24, 2019 13:51

practicalswift force-pushed the globals-1 branch from 43b8d89 to fb43415 Compare May 25, 2019 21:23

maflcko merged commit fb43415 into bitcoin:master May 25, 2019

practicalswift mentioned this pull request Jun 3, 2019

Don't use global (external) symbols for symbols that are used in only one translation unit #16092

Merged

practicalswift deleted the globals-1 branch April 10, 2021 19:38

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

practicalswift commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

promag commented Mar 20, 2019

Uh oh!

maflcko commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Mar 21, 2019

Uh oh!

Uh oh!

practicalswift commented Mar 22, 2019

Uh oh!

promag commented Mar 22, 2019

Uh oh!

lucayepa commented Mar 23, 2019

Uh oh!

practicalswift commented Mar 27, 2019

Uh oh!

Empact commented Apr 3, 2019

Uh oh!

Uh oh!

sipa commented Apr 16, 2019

Uh oh!

practicalswift commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented May 7, 2019

Uh oh!

ajtowns commented May 22, 2019

Uh oh!

practicalswift commented May 24, 2019

Uh oh!

Uh oh!

DrahtBot commented May 24, 2019

Conflicts

Uh oh!

maflcko commented May 25, 2019

Uh oh!

practicalswift commented May 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

practicalswift commented Mar 19, 2019 •

edited

Loading

maflcko commented Mar 20, 2019 •

edited

Loading

practicalswift commented Apr 16, 2019 •

edited

Loading