Remove global symbols: Avoid using the global namespace if possible #15622

practicalswift commented at 8:43 AM on March 19, 2019: contributor

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

fanquake added the label Refactoring on Mar 19, 2019

in src/httpserver.cpp:153 in 596626c725 outdated

 154 | +namespace {
 155 | +    //! Handlers for (sub)paths
 156 | +    std::vector<HTTPPathHandler> pathHandlers;
 157 | +    //! Bound listening sockets
 158 | +    std::vector<evhttp_bound_socket *> boundSockets;
 159 | +}

MarcoFalke commented at 1:36 PM on March 19, 2019:

Why can't you wrap everything above into a namespace? Also should not change the indentation. I'd suggest to use clang-format

practicalswift commented at 3:16 PM on March 19, 2019:

Good point! Fixed!

With this change HTTPPathHandler, WorkQueue and HTTPWorkItem are no longer global.

Please re-review!

practicalswift force-pushed on Mar 19, 2019

promag commented at 6:22 PM on March 20, 2019: member

Concept ACK, change looks good.

MarcoFalke commented at 9:21 PM on March 20, 2019: member

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

Empact commented at 8:14 AM on March 21, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15622/commits/b112097f8e5f96e96db47e2eef9870d6f0431677

in src/init.cpp:78 in b112097f8e outdated

  70 | @@ -71,13 +71,15 @@
  71 |  #include <zmq/zmqrpc.h>
  72 |  #endif
  73 |  
  74 | +namespace {
  75 |  bool fFeeEstimatesInitialized = false;
  76 |  static const bool DEFAULT_PROXYRANDOMIZE = true;

promag commented at 3:02 PM on March 21, 2019:

I think you could drop static?

practicalswift commented at 3:05 PM on March 21, 2019:

Yes, absolutely but in order to make reviewing super trivial I didn't want to change any existing lines. Let me know if that decision is blocking an utACK from you and I'll consider reconsidering :-)

promag commented at 3:06 PM on March 21, 2019:

I don't think it'd make this non-trivial. I vote to remove static where possible inside anonymous namespace. Wait until others weight in.

Empact commented at 9:29 AM on March 22, 2019:

With or without the declaration seems they'll have internal linkage (via unnamed namespace) and static storage duration (via namespace scope).

Mildly in favor of removal on the grounds that a distinction without a difference in this case can be misleading.

static When used in a declaration of an object, it specifies static storage duration. When used in a declaration at namespace scope, it specifies internal linkage. static storage duration. All objects declared at namespace scope (including global namespace) have this storage duration, plus those declared with static or extern. https://en.cppreference.com/w/cpp/language/storage_duration

Unnamed namespace definition. Its members have potential scope from their point of declaration to the end of the translation unit, and have internal linkage. https://en.cppreference.com/w/cpp/language/namespace#Unnamed_namespaces

practicalswift commented at 9:39 AM on March 22, 2019: contributor

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

Please re-review :-)

promag commented at 2:50 PM on March 22, 2019: member

utACK 9638da7.

lucayepa commented at 11:52 PM on March 23, 2019: contributor

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](/bitcoin-bitcoin/15465/).

practicalswift commented at 8:28 PM on March 27, 2019: contributor

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

Empact commented at 5:28 AM on April 3, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15622/commits/9638da76386ba1e6f610f26a5bc9b88825087bdc

in src/init.cpp:82 in 9638da7638 outdated

  81 | +const bool DEFAULT_STOPAFTERBLOCKIMPORT = false;
  82 |  
  83 |  // Dump addresses to banlist.dat every 15 minutes (900s)
  84 | -static constexpr int DUMP_BANS_INTERVAL = 60 * 15;
  85 | +constexpr int DUMP_BANS_INTERVAL = 60 * 15;
  86 | +} // namespace

MarcoFalke commented at 8:29 PM on April 16, 2019:

Why is this changed? What is the difference between static const(expr) int and const int in a namespace?

We don't change code for stylistic reasons.

MarcoFalke commented at 8:31 PM on April 16, 2019:

Also, I cant see a reason to not have literals in the global namespace

sipa commented at 8:32 PM on April 16, 2019:

static inside a anonymous namespace is redundant.

practicalswift commented at 8:48 PM on April 16, 2019:

Please see also #15622 (review)

Let me know if you find any reason to revert to the original version.

practicalswift commented at 9:05 PM on April 16, 2019:

Also, I cant see a reason to not have literals in the global namespace

If we wanted to share say DUMP_BANS_INTERVAL across more than one translation unit the way to do it is via inclusion of a it via say init.h, no?

MarcoFalke commented at 9:40 PM on April 16, 2019:

static inside a anonymous namespace is redundant.

ftfy: anonymous namespace is redundant when the constexpr is already static

sipa commented at 8:35 PM on April 16, 2019: member

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

practicalswift renamed this:
~~Remove globals: Avoid using the global namespace if possible~~
Remove global symbols: Avoid using the global namespace if possible
on Apr 16, 2019

practicalswift commented at 8:46 PM on April 16, 2019: contributor

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

practicalswift commented at 4:59 PM on May 7, 2019: contributor

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 commented at 6:36 AM on May 22, 2019: member

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 force-pushed on May 24, 2019

practicalswift commented at 1:21 PM on May 24, 2019: contributor

@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? :-)

in src/warnings.cpp:11 in ec7ee881f1 outdated

   7 | @@ -8,10 +8,10 @@
   8 |  #include <util/system.h>
   9 |  #include <warnings.h>
  10 |  
  11 | -CCriticalSection cs_warnings;
  12 | -std::string strMiscWarning GUARDED_BY(cs_warnings);
  13 | -bool fLargeWorkForkFound GUARDED_BY(cs_warnings) = false;
  14 | -bool fLargeWorkInvalidChainFound GUARDED_BY(cs_warnings) = false;
  15 | +static CCriticalSection cs_warnings;

MarcoFalke commented at 1:36 PM on May 24, 2019:

style nit:

static RecursiveMutex cs_warnings;

practicalswift commented at 1:49 PM on May 24, 2019:

Done!

practicalswift force-pushed on May 24, 2019

DrahtBot commented at 6:14 PM on May 24, 2019: member

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.

MarcoFalke commented at 2:36 PM on May 25, 2019: member

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

Remove global symbols: Avoid using the global namespace if possible

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

fb434159d1

practicalswift force-pushed on May 25, 2019

practicalswift commented at 9:24 PM on May 25, 2019: contributor

@MarcoFalke Done! Please re-review :-)

MarcoFalke merged this on May 25, 2019

MarcoFalke closed this on May 25, 2019

MarcoFalke referenced this in commit 8ab4f282c0 on May 25, 2019

deadalnix referenced this in commit dfd3616955 on Jun 11, 2020

practicalswift deleted the branch on Apr 10, 2021

PastaPastaPasta referenced this in commit 9936bfe112 on Jun 27, 2021

PastaPastaPasta referenced this in commit 2184ba19ed on Jun 28, 2021

PastaPastaPasta referenced this in commit 51c46d0edb on Jun 29, 2021

PastaPastaPasta referenced this in commit 225c8690ba on Sep 11, 2021

PastaPastaPasta referenced this in commit 3f346d2fa9 on Sep 11, 2021

PastaPastaPasta referenced this in commit 74a2557245 on Sep 12, 2021

DrahtBot locked this on Aug 16, 2022