Proper namespace usage redux

782 views
Skip to first unread message

Peter Kasting

unread,
Nov 27, 2017, 8:04:34 PM11/27/17
to Chromium-dev
TLDR:

I propose that we try to keep global-scope functions in some kind of class or namespace.

Detailed proposal:

Try to avoid global-scope functions.  Prefer to make these members of a namespace or static methods on a class.  Prefer adding namespaces to adding "namespace-like" classes that exist only to scope static methods (see http://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions ).

Downsides/counterarguments:

"Proliferating arbitrary ad-hoc namespaces is an invitation to be inconsistent; how do people know when to use an existing namespace or make their own, and what to name it?"

Counter-counterargument: People are already expected to add functionality to existing classes/APIs where it makes sense; expecting them to add to an existing namespace where appropriate doesn't seem harder.  Similarly, for naming, we already let people specify their own names for classes, functions, etc. and expect them to be reasonable; how is this different?

"There's little utility for this; if names are descriptive enough and avoid symbol conflicts, what benefit do we gain from further namespacing?"

Counter-counterargument: This is already a compromise with the "namespace almost everything" policy of the style guide; it reduces the chance for symbol conflicts; it can reduce verbosity within the files that implement these methods [if function/object names are shortened to avoid overlap with the namespace name]; and it helps readers when dissimilarly-named but conceptually-related functions share a namespace.

Examples:

https://chromium-review.googlesource.com/c/chromium/src/+/789090 removes the chrome:: namespace from some otherwise-global-scope functions.  Per the above policy it would be nice to keep these in a namespace, so for example:
Background info:

In https://groups.google.com/a/chromium.org/d/topic/chromium-dev/X_MSSCmNsWE/discussion and then https://groups.google.com/a/chromium.org/d/topic/chromium-dev/pcgYLSJsii8/discussion , we decided to remove the generic "chrome::" namespace.  There was some hostility expressed to the idea of adding/proliferating too many other ad-hoc namespaces.

However, the Google Style Guide says, "With few exceptions, place code in a namespace."  It leaves name construction discretion largely to authors.

On https://chromium-review.googlesource.com/773522 , James Cook noted " if there are bare functions in the chrome:: namespace, I would prefer that they go into a utility class/namespace at the same time that they come out of the chrome:: namespace."

The "Jumbo builds" work has run into symbol conflicts, although I think that typically happens with file-scope symbols and not global-scope ones.

PK

Peter Kasting

unread,
Nov 27, 2017, 8:17:58 PM11/27/17
to Chromium-dev
On Mon, Nov 27, 2017 at 5:03 PM, Peter Kasting <pkas...@google.com> wrote:
Examples:

https://chromium-review.googlesource.com/c/chromium/src/+/789090 removes the chrome:: namespace from some otherwise-global-scope functions.  Per the above policy it would be nice to keep these in a namespace, so for example:
Incidentally, another possibility is to use fewer, broader namespaces, e.g. introduce a browser_navigation:: namespace that all the above (and other stuff) would live in.  The problem here is that it doesn't let you reduce the verbosity of any of the existing names, so it lessens one of the benefits of doing this.

Also, since I didn't make it clear, I very much want feedback on this proposal, especially if people still think it's explicitly better to just put things in the global scope.  There are a lot of "remoev chrome:: namespace" CLs flying around lately and we should have consensus on how to deal with them going forward before moving much further.

PK

Brett Wilson

unread,
Nov 27, 2017, 11:26:16 PM11/27/17
to Peter Kasting, Chromium-dev
I sympathize in spirit, but I don't think it's practical to have a meaningful change in this area. I tried to enforce better namespace usage when the codebase and team were much smaller, and failed. I think it's too hard to enforce across such a large team unless we had a super rigid definition that can be automatically checked of what every namespace should be. Changing the existing usage is a herculean task with relatively little upside.

In the end, I think people should give good names to things. Namespaces are one way to do that but not the only way. If people give good names to things it doesn't really matter exactly how, and if people give bad names to things namespaces won't help.

If we want to make some progress, I would suggest finding enums in the global namespace (or maybe the common big ones like base and content) and changing them to well-named enum classes. There are some dubious ones so the benefit is greater, and it's not super hard. For example, I ran into an enum value of "TAB" in the global namespace today:

Brett

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFA1Woi1P%3Di-%2Bt1HKRecY8Gouv0AzdpLKFLhu3JLejY7BA%40mail.gmail.com.

Peter Kasting

unread,
Nov 28, 2017, 1:44:16 AM11/28/17
to Brett Wilson, Chromium-dev
On Mon, Nov 27, 2017 at 8:25 PM, Brett Wilson <bre...@chromium.org> wrote:
I sympathize in spirit, but I don't think it's practical to have a meaningful change in this area. I tried to enforce better namespace usage when the codebase and team were much smaller, and failed. I think it's too hard to enforce across such a large team unless we had a super rigid definition that can be automatically checked of what every namespace should be. Changing the existing usage is a herculean task with relatively little upside.

I'm not worried about enforcement or mass-changing the existing usage.  If we have a recommended practice, I'll ask for it in the reviews I'm doing on removing all these chrome:: namespace btis, since in practice it seems like I'm doing them all for some reason.

This is in keeping with how we've done most other style and Dos-and-Donts changes, which is to simply make the new recommendation and not worry about forcing the existing codebase to match.

I'm curious how you feel about the proposal given that implementation plan.

In the end, I think people should give good names to things. Namespaces are one way to do that but not the only way. If people give good names to things it doesn't really matter exactly how, and if people give bad names to things namespaces won't help.

Is this an argument that even in the abstract this isn't worth recommending?

If we want to make some progress, I would suggest finding enums in the global namespace (or maybe the common big ones like base and content) and changing them to well-named enum classes. There are some dubious ones so the benefit is greater, and it's not super hard. For example, I ran into an enum value of "TAB" in the global namespace today:

While that may be of benefit, it's not an area I'm interested in right now.  The practical issue is that we're about to dump everything that was in chrome:: into the global namespace and I need to know what the team wants to do about it.

PK 

Adam Rice

unread,
Nov 28, 2017, 3:23:55 AM11/28/17
to Chromium-dev
If I understand correctly, a major argument in 2013 for dropping the chrome:: namespace is that there was no way to fix existing usage. But it's 2017 now. We have the technology.

I rarely touch stuff in //chrome, but when I do I find it weird that everything is just dumped in the global namespace.

I'm just saying this because no-one's explicitly mentioned it, but a major issue with the global namespace is that all those symbols show up in every other namespace. This can lead to surprising behaviour when header files are missing.

I don't feel much of a need for fine-grained namespaces. Blink has 500k lines of code, mostly under the blink:: namespace. However, I don't mind them either. There's hundreds of namespaces in //components but I've never felt it was problem. I wouldn't want there to be a rule like "files under //net/http have to use the net_http namespace" but I assume people would exercise moderation. Namespace net::http_util would be better than class net::HttpUtil.

Peter Kasting

unread,
Nov 28, 2017, 3:25:26 AM11/28/17
to Adam Rice, Brett Wilson, Chromium-dev
On Tue, Nov 28, 2017 at 12:21 AM, Adam Rice <ri...@google.com> wrote:
If I understand correctly, a major argument in 2013 for dropping the chrome:: namespace is that there was no way to fix existing usage.

The major argument was that it added no clarity, was so broad as to add basically no safety, and extending it to cover the entire product would buy nothing but churn and verbosity.

Finer-grained namespaces could improve on the clarity and safety aspects without implying that we should extend them more broadly.
 
I'm just saying this because no-one's explicitly mentioned it, but a major issue with the global namespace is that all those symbols show up in every other namespace. This can lead to surprising behaviour when header files are missing.

I don't know what the last sentence refers to.

PK

Adam Rice

unread,
Nov 28, 2017, 3:37:33 AM11/28/17
to Peter Kasting, Brett Wilson, Chromium-dev
On 28 November 2017 at 17:24, Peter Kasting <pkas...@google.com> wrote:
I don't know what the last sentence refers to.

Sorry, I should have been explicit.

I was talking about the situation where you have two symbols with the same name, one in your namespace and one in the global namespace. You attempt to reference the your own symbol, but because you forget the #include you get the global symbol instead.

If no code that used namespaces ever #included header files that don't use namespaces then this wouldn't arise, but we don't have that guarantee in practice.

Colin Blundell

unread,
Nov 28, 2017, 4:22:09 AM11/28/17
to ri...@chromium.org, Peter Kasting, Brett Wilson, Chromium-dev
Personally, I strongly dislike namespaces that can't be inferred by the location of the file. I understand our current best practices wrt namespacing as follows:

- If you're in a top-level embedder (//chrome, //ios/chrome, ...), don't use a namespace.
- Otherwise, use the namespace of the module that you're in (which is typically highly predictable, e.g., //content/<foo> -> ::content, //components/<foo>, -> ::foo, etc).

(Of course, we have lots of existing violations of the above, but in this thread we're talking about best practices going forward).

Note that for free functions not in a top-level embedder, there should already be a namespace to use. Hence, we're here talking only about free functions in top-level embedders (as you reference by explicitly putting this discussion in the context of removing the ::chrome namespace). For those free functions, I would strongly prefer that we be consistent with our current best practice of "don't use a namespace in top-level embedders" rather than modify that to "don't use a namespace in top-level embedders unless it's a free function, in which case make up a reasonable namespace if there's really no better place to put the free function."

Best,

Colin

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Daniel Bratell

unread,
Nov 28, 2017, 12:20:09 PM11/28/17
to Chromium-dev, pkas...@google.com
On Tue, 28 Nov 2017 02:03:16 +0100, 'Peter Kasting' via Chromium-dev <chromi...@chromium.org> wrote:

The "Jumbo builds" work has run into symbol conflicts, although I think that typically happens with file-scope symbols and not global-scope ones.

Yes, this is slightly different.  We get problems with anonymous namespaces that by definition are specific to the translation unit ("cc file plus everything included"). In jumbo many cc files share the same translation unit and will share the same anonymous namespace.

Using more explicit namespaces has been an easy and maintainable way to get around collisions but I don't think it should have any impact on whether exported/visible symbols should be in namespaces or not.

One lesson learned in jumbo though (and covered by the style guide already): Never use a name for a sub-namespace that is also a top-level namespace. So please, no "net", "testing", "url", "mojo", ... as names for inner namespaces.

/Daniel


--
/* Opera Software, Linköping, Sweden: CET (UTC+1) */

James Cook

unread,
Nov 28, 2017, 2:16:31 PM11/28/17
to Daniel Bratell, Chromium-dev, Peter Kasting
I support this proposal, particularly for utility functions. I prefer namespaces that match filenames, but static classes work too. It helps me infer the filename of a utility function definition from each call site.

For example, compare this existing call to GetDisplayUsername:

  auto label = base::MakeUnique<views::Label>(
      GetDisplayUsername(form), CONTEXT_BODY_TEXT_LARGE, STYLE_SECONDARY);

versus:

  auto label = base::MakeUnique<views::Label>(
      ManagePasswordsViewUtils::GetDisplayUsername(form), CONTEXT_BODY_TEXT_LARGE, STYLE_SECONDARY);

or:

  auto label = base::MakeUnique<views::Label>(
      manage_passwords_view_utils::GetDisplayUsername(form), CONTEXT_BODY_TEXT_LARGE, STYLE_SECONDARY);

The first case requires searching the existing file to look for methods and anonymous namespace functions, then a trip through code search to find the definition. In the latter 2 cases it's trivial to recognize that it's a bare function not in the existing class or file and guess the filename. Also, many editors support directly opening the file by name.

I do not like the idea of introducing namespaces that neither match top-level components nor sub-component filenames. For example, navigate::Navigate(foo) doesn't help me to discover that the definition is in chrome/browser/ui/browser_navigator.cc

James

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Peter Kasting

unread,
Nov 28, 2017, 3:12:04 PM11/28/17
to James Cook, Daniel Bratell, Chromium-dev
On Tue, Nov 28, 2017 at 11:15 AM, James Cook <jame...@chromium.org> wrote:
I do not like the idea of introducing namespaces that neither match top-level components nor sub-component filenames. For example, navigate::Navigate(foo) doesn't help me to discover that the definition is in chrome/browser/ui/browser_navigator.cc

Hmm.  We have identical filenames in many directories, and we have similar functions (e.g. functions related to navigation) whose implemented are spread across a variety of files.  I'm not sure I can support the idea of "make the namespace match the filename if it isn't part of a top-level component".

Given that your reply is the only one favorable to this proposal so far, and what you actually want isn't something I really support :), I'm leaning towards withdrawing this proposal, and letting Colin Blundell's description of best practices (which I think is accurate) stand; but I will wait another day or two for further debate.

PK 

Peter Kasting

unread,
Dec 1, 2017, 3:50:38 PM12/1/17
to James Cook, Daniel Bratell, Chromium-dev
On Tue, Nov 28, 2017 at 12:10 PM, Peter Kasting <pkas...@google.com> wrote:
Given that your reply is the only one favorable to this proposal so far, and what you actually want isn't something I really support :), I'm leaning towards withdrawing this proposal, and letting Colin Blundell's description of best practices (which I think is accurate) stand; but I will wait another day or two for further debate.

Hearing nothing further, I withdraw this proposal.

Current recommended practice (copied from Colin's earlier mail):

- If you're in a top-level embedder (//chrome, //ios/chrome, ...), don't use a namespace.
- Otherwise, use the namespace of the module that you're in (which is typically highly predictable, e.g., //content/<foo> -> ::content, //components/<foo>, -> ::foo, etc).

Thanks for the input all.

PK

Michael Giuffrida

unread,
Dec 1, 2017, 4:15:37 PM12/1/17
to pkas...@google.com, James Cook, Daniel Bratell, Chromium-dev
It sounds like there was at least one related best practice, then:

- Prefer enum classes instead of enums in the global namespace

What about constants in top-level embedders? Chrome URL constants are currently in the chrome namespace -- if we move them to the top-level namespace, it gets harder to figure out where a constant is from (some header file? an anonymous namespace in the current file?) and adds quite a lot of top-level names.

As another example, chrome/browser/media's "kUploadUrl" was recently moved out of the chrome namespace. Predictably, this clashes with other constants as in this anonymous namespace. Is there a recommended way of ensuring constant names are specific enough to prevent this?

Michael (resent from correct account this time)

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Peter Kasting

unread,
Dec 1, 2017, 4:39:30 PM12/1/17
to Michael Giuffrida, James Cook, Daniel Bratell, Chromium-dev
On Fri, Dec 1, 2017 at 1:13 PM, Michael Giuffrida <mich...@chromium.org> wrote:
It sounds like there was at least one related best practice, then:

- Prefer enum classes instead of enums in the global namespace

I think that's a reasonable suggestion.  It's not really our existing "recommended practice", so if we really want people to do this, might be best to raise as a proposal thread (are we trying to update the team style guide, or just the dos and donts page)?

What about constants in top-level embedders? Chrome URL constants are currently in the chrome namespace -- if we move them to the top-level namespace, it gets harder to figure out where a constant is from (some header file? an anonymous namespace in the current file?) and adds quite a lot of top-level names.

Honestly, I never found "chrome::" on these very informative -- it always just seemed verbose and unhelpful.  I don't actually view this world as much worse than the current one (modulo my next comment).
 
As another example, chrome/browser/media's "kUploadUrl" was recently moved out of the chrome namespace. Predictably, this clashes with other constants as in this anonymous namespace. Is there a recommended way of ensuring constant names are specific enough to prevent this?

This is a case where we really should have named the constant better (e.g.  kWebRtcLogUploadUrl).  Or even better, just inlined it into the one function that uses it instead of giving it its own module (!) -- https://chromium-review.googlesource.com/c/chromium/src/+/804147 .

In general I think we ought to be OK if people ensure things are named "sufficiently descriptively" (use your judgment, and fix bad cases if you see them).  (It also helps when people scope constants locally where possible.)

PK

Alexey Baskakov

unread,
Sep 5, 2018, 12:32:46 AM9/5/18
to Chromium-dev, jame...@chromium.org, bra...@opera.com
Hi all!

Sorry for popping up this thread again...

On Saturday, December 2, 2017 at 7:50:38 AM UTC+11, Peter Kasting wrote:
On Tue, Nov 28, 2017 at 12:10 PM, Peter Kasting wrote:
Given that your reply is the only one favorable to this proposal so far, and what you actually want isn't something I really support :), I'm leaning towards withdrawing this proposal, and letting Colin Blundell's description of best practices (which I think is accurate) stand; but I will wait another day or two for further debate.

Hearing nothing further, I withdraw this proposal.

Current recommended practice (copied from Colin's earlier mail):

- If you're in a top-level embedder (//chrome, //ios/chrome, ...), don't use a namespace.
- Otherwise, use the namespace of the module that you're in (which is typically highly predictable, e.g., //content/<foo> -> ::content, //components/<foo>, -> ::foo, etc).

Can you clarify "a top-level embedder" term?
Should everything under src/chrome/ be in a global namespace?

For example:
1) We have src/apps/ directory.
2) Some apps-related code (which uses profile.h) lives in src/chrome/browser/apps/ 
3) So /apps/ is a part of chrome/
Should apps:: namespace be avoided in favor of global namespace?

Thanks in advance for any clarifications.
 

Peter Kasting

unread,
Sep 5, 2018, 12:52:52 AM9/5/18
to lo...@chromium.org, Chromium-dev, James Cook, Daniel Bratell
Your (3) seems off to me: just because some bits related to apps are in chrome/ does not mean all of apps/ is in chrome/.

I don't think the rules are very clear-cut.  You can probably use or not use an apps:: namespace depending on what you think is clearer.  Just don't do something like chrome::apps:: for the bits in chrome/, and don't do repetitious names like apps::AppsXYZ.

PK

Michael Giuffrida

unread,
Sep 5, 2018, 1:29:46 AM9/5/18
to pkas...@chromium.org, lo...@chromium.org, Chromium-dev, James Cook, Daniel Bratell
On Tue, Sep 4, 2018 at 9:52 PM Peter Kasting <pkas...@chromium.org> wrote:
On Tue, Sep 4, 2018 at 9:32 PM Alexey Baskakov <lo...@chromium.org> wrote:
On Saturday, December 2, 2017 at 7:50:38 AM UTC+11, Peter Kasting wrote:
On Tue, Nov 28, 2017 at 12:10 PM, Peter Kasting wrote:
Given that your reply is the only one favorable to this proposal so far, and what you actually want isn't something I really support :), I'm leaning towards withdrawing this proposal, and letting Colin Blundell's description of best practices (which I think is accurate) stand; but I will wait another day or two for further debate.

Hearing nothing further, I withdraw this proposal.

Current recommended practice (copied from Colin's earlier mail):

- If you're in a top-level embedder (//chrome, //ios/chrome, ...), don't use a namespace.
- Otherwise, use the namespace of the module that you're in (which is typically highly predictable, e.g., //content/<foo> -> ::content, //components/<foo>, -> ::foo, etc).

What's a top-level embedder?

Is content_shell a top-level embedder? (Nothing else embeds it; it's a standalone application.) Should code in //content/shell be in the global namespace instead of the content or content::shell namespace?

Ditto for app shell in //extensions/shell, and cast shell in //chromecast.

For some motivating examples, here are the class names for the equivalent of ChromeContentBrowserClient in these embedders:

content::ShellContentBrowserClient
extensions::ShellContentBrowserClient
chromecast::shell::CastContentBrowserClient
 

Can you clarify "a top-level embedder" term?
Should everything under src/chrome/ be in a global namespace?

For example:
1) We have src/apps/ directory.
2) Some apps-related code (which uses profile.h) lives in src/chrome/browser/apps/ 
3) So /apps/ is a part of chrome/
Should apps:: namespace be avoided in favor of global namespace?

Thanks in advance for any clarifications.

Your (3) seems off to me: just because some bits related to apps are in chrome/ does not mean all of apps/ is in chrome/.

I don't think the rules are very clear-cut.  You can probably use or not use an apps:: namespace depending on what you think is clearer.  Just don't do something like chrome::apps:: for the bits in chrome/, and don't do repetitious names like apps::AppsXYZ.

PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Colin Blundell

unread,
Sep 7, 2018, 9:40:28 AM9/7/18
to mich...@chromium.org, pkas...@chromium.org, lo...@chromium.org, Chromium-dev, James Cook, Daniel Bratell
Personally, I think that it's most straightforward to think of the shell for a given module as being "in that module", which would put it in the second rule and also leaves the second rule as consistent as possible (i.e., otherwise there would be e.g. subdirectories of //content that aren't in the ::content namespace). This is definitely a murky and debatable area though.

//chromecast seems like a top-level embedder period from my naive POV, so I would think that ::chromecast is unnecessary.

I don't think that the nested ::shell namespace is ever necessary.

Alexey Baskakov

unread,
Oct 9, 2018, 10:51:03 PM10/9/18
to Chromium-dev, lo...@chromium.org, jame...@chromium.org, bra...@opera.com
Hi, Peter!

On Wednesday, September 5, 2018 at 2:52:52 PM UTC+10, Peter Kasting wrote:
Avoiding repetitious names (like using foo::Registry instead of foo::FooRegistry) forces you to use short file names (registry.cc instead of foo_registry.cc)
As a result, we can have several registry.cc in the project.
This might be a problem since most text editors and IDEs represent "Open Source File" UI as a global flat list of all files.
Enforcing the foo::Registry <-> foo/registry.cc isomorphism doesn't look realistic.

Alexey.

PK

Peter Kasting

unread,
Oct 10, 2018, 10:24:02 AM10/10/18
to lo...@chromium.org, Chromium-dev, James Cook, Daniel Bratell
On Tue, Oct 9, 2018 at 7:51 PM Alexey Baskakov <lo...@chromium.org> wrote:
Avoiding repetitious names (like using foo::Registry instead of foo::FooRegistry) forces you to use short file names (registry.cc instead of foo_registry.cc)

I don't think that's a rule.  You can name your file foo_registry.cc if you really want, even if the class is just "Registry".  Lots of Chrome UI code does stuff like this.

As a result, we can have several registry.cc in the project.
This might be a problem since most text editors and IDEs represent "Open Source File" UI as a global flat list of all files.

I don't think we should try to optimize for this.  Sacrificing code brevity for the sake of someone's file-picker UI seems like the wrong tradeoff, and it's not as if we'll namespace every possible case where files could have the same name across the project anyway, especially once third-party and system code starts entering into the mix.

PK
Reply all
Reply to author
Forward
0 new messages