cxx: Allow namespaces in //chrome/browser.

[Erik Chen]

Hello cxx,

The OWNERS of //chrome have put together design principles for //chrome/browser to make it easier to build and maintain code (internal discussion).

One of these design principles (modular features) allows directories to have their own namespace. There is currently guidance from cxx@ that disallows namespaces in //chrome. I attempted to remove this guidance, but was informed that I first need consensus from this mailing list.

[Peter Kasting]

On Thursday, May 9, 2024 at 8:44:28 PM UTC-7 Erik Chen wrote:

One of these design principles (modular features) allows directories to have their own namespace. There is currently guidance from cxx@ that disallows namespaces in //chrome. I attempted to remove this guidance, but was informed that I first need consensus from this mailing list.

TLDR: Can you clarify the problems with the status quo, and how this would be a clear improvement? Changes here might be challenging.

* Longer version follows *

History. The Google Style Guide advises using namespaces pervasively: https://google.github.io/styleguide/cppguide.html#Namespaces.  However, this is also in a different context. There are many projects in Google's repo; there is effectively one in Chrome's.

In the spirit of the Google style guide, we initially tried to standardize on at least having a namespace for most top-level-dirs, which matched the name of that dir. For some directories (like base/ and content/) this is still the case. However, we ran into problems:

• Because so much code was in chrome/ and because chrome/ was intended to be a leaf not consumed elsewhere, the cost/benefit of a "chrome::" namespace everywhere in chrome/ didn't seem worth it (see e.g. https://groups.google.com/a/chromium.org/g/chromium-dev/c/X_MSSCmNsWE/m/h7GMRuKvp2AJ and subsequent messages on that thread by me and jam@).
• Once we created components/, it felt inappropriate to namespace all that code as "components::", but verbose to name everything "components::foobar::" (though that would have been the google3 route). People ended up largely using component names as the namespace names, e.g. "payments::" in components/payments/.
• At that point, adding more namespaces inside chrome/ risked name clashes. For a variety of reasons, people began recommending that reusable code move to a components/ subdir (see e.g. https://groups.google.com/a/chromium.org/g/chromium-dev/c/pcgYLSJsii8/m/yIxZL8ipG-gJ), which sidestepped this issue. (In the early days, components/ had significance in terms of iOS use and where in the dependency layer hierarchy the code fell. That was subsequently abandoned, so that now the rules are weaker; see https://source.chromium.org/chromium/chromium/src/+/main:components/README.md.)
• Eventually, darin@, jam@, and others publicly opposed ad-hoc namespaces (see e.g. https://groups.google.com/a/chromium.org/g/chromium-dev/c/pcgYLSJsii8/m/g_QmEAuhxJAJ), and existing practice (which by that point rarely used them) calcified.
• About seven years ago I proposed greater openness to namespaces (see https://groups.google.com/a/chromium.org/g/chromium-dev/c/8ROncnL1t4k/m/RJrNPBHfAwAJ). This did not get traction.
• Since then, a few more patterns have crept in (e.g. "features::" for feature flags), I believe mostly due to unfamiliarity with the Chrome style guidance.

Questions.

• Can these features live in components/? If they are only used once, then components/README.md pretty clearly says "no". But maybe some of them would be used from multiple places (e.g. different UI implementations)? If so, they would be candidates to move there.
• If not: how would we choose the namespace names? We don't have chrome:: or browser:: (and probably don't want to add either), so a google3-compliant `chrome::browser::foobar::` seems out. But using just `foobar::` seems likely to collide with code in components/foobar/. You could argue "that's a feature, not a bug" if those two dirs are part of the same larger feature concept. But having a single namespace, whose contents live in completely diverged places in the tree, seems quite bad to me. We could use `c_b_foobar`, but I suspect developers will quickly rebel against that.
• What about other code? Simply removing the namespace guidance entirely means that the standing rule is the google3 rule, which no code in Chromium obeys, or likely ever wants to obey. So I think that's out. How do you want to codify the updated guidance such that it covers everywhere?

Possibilities.

1. Status quo. If the code in these dirs isn't causing symbol conflicts with our existing, horribly tangled build files full of circular dependencies, it seems even less likely to if we clean those up and modularize things. Low benefit for the cost.
2. Some container namespace for modularized features. "chrome_browser::" restates the path, but is also verbose (and suggests that everything in c/b/, modular and not, should live there). "modular_feature::"? "modular::"? Is there a short, unique, clear way of referring to the collective of all these features?
3. Perhaps in conjunction with (2), a different top-level dir for these features.

At the moment I'd vote for (1), but I could be convinced otherwise.

PK

[danakj]

I'm not sure if this is here nor there, but in some future we'd hope to have a better module'd codebase. Having namespaces span across multiple modules in C++ is possible but seems very confusing and undesirable. In Rust it is not possible.

It seems to me that the long term direction is better namespace scoping, including having a separate namespace for content_browser and content_renderer, rather than shoving them all into one content namespace/module and using DEPS files to try prevent includes (which keeps failing to do so, and won't work for modules in either language). Same for chrome_browser and chrome_renderer (or just browser and renderer perhaps).

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/5eceeded-593e-485c-a68e-baf87e8ab5fcn%40chromium.org.

[Jean-Philippe Gravel]

> We don't have chrome:: or browser:: (and probably don't want to add either), so a google3-compliant `chrome::browser::foobar::` seems out.

The current Google3 guidance (https://abseil.io/tips/130) actually recommends against namespace nesting and against naming namespaces based on folder paths. This is because of how argument dependent lookup works in C++. Quoting this article:

The fundamental problem for building namespaces out of packages is that we rarely rely on fully-qualified lookup in C++, normally writing std::unique_ptr rather than ::std::unique_ptr. Coupled with lookup in enclosing namespaces, this means that for code in a deeply nested package (::division::section::team::subteam::project, for example) any symbol that is not fully qualified (std::unique_ptr) can in fact reference any of

• ::std::unique_ptr
• ::division::std::unique_ptr
• ::division::section::std::unique_ptr
• ::division::section::team::std::unique_ptr
• ::division::section::team::subteam::std::unique_ptr
• ::division::section::team::subteam::project::std::unique_ptr

Then:

Practically speaking, the following is the best we can do given the realities of most codebases:

• Have a database of some form for a codebase to identify the unique namespaces.
• When introducing a new namespace, use that database and introduce it as a top-level.
• If for some reason the above is impossible, never ever introduce a sub-namespace that matches a well-known top-level namespace. No sub-namespaces for absl, testing, util, etc. Try to give sub-namespaces unique names that are unlikely to collide with future top-levels.
• When declaring namespace aliases and using-declarations, use fully qualified names, unless you are referring to a name inside the current namespace, as per TotW 119.
• For code in util or other commonly-abused namespaces, try to avoid full qualification, but qualify if necessary.

In other words, even if you were to use `chrome::browser::foobar::`, you would need to make sure that `foobar` isn't used elsewhere in the code base. At that point, `chrome::browser::` wouldn't provide much value and you'd be better off just naming your namespace `foobar`. What we need though is a database strategy for ensuring that namespaces are unique.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSvkwgcfW%3DCK9F-zJpKO9KBiVXh0Nj534FhvFF2rXB22g%40mail.gmail.com.

[Erik Chen]

Hi Peter,

I don't think we're on the same page here. I've answered your questions below for completeness. If you think it's important I'm happy to take a VC.

To summarize:

* From my perspective, being able to use namespaces is a helpful organization tool. Alongside some of the other design principles, it's a forcing function that causes authors to write modular features, with well defined API boundaries. This will begin to solve some of the larger architectural issues with //chrome.

* Based on your questions and comments above, it seems like you're interested in a machine-automatable, not-too-verbose, conflict-free mechanism for defining namespaces. Since no one has been able to come up with a system to do so, the default guidance until now is to not use namespaces.

> TLDR: Can you clarify the problems with the status quo, and how this would be a clear improvement? Changes here might be challenging.

The broader problem is that "features" in //chrome are not well defined, and do not have clear API surfaces. They often involve scattering functions/classes across a variety of files. Implementation details for multiple features are often mingled together. This results in code that is hard to test, spooky action at a distance, unexpected re-entrancy, and other problems.

> Can these features live in components/? If they are only used once, then components/README.md pretty clearly says "no". But maybe some of them would be used from multiple places (e.g. different UI implementations)? If so, they would be candidates to move there.

This is not the right question to ask. The problem today is we don't have clearly delineated features with well defined APIs/dependencies. If we had features, whether the features live in //components or //chrome is a smaller concern we can address later.

> If not: how would we choose the namespace names?

Use best judgement. It's not the name that matters, it's the conceptual organization of the code. Most of the concerns about nomenclature seem to be about verbosity. This is a symptom of the underlying problem: lack of clear (and limited) API surfaces. If a feature only has 2 public APIs, it frankly doesn't matter whether the namespace is 5 or 20 characters. Code inside the feature will use a namespace <foo> {} block, and code outside the feature will use foo::PublicAPI(), which is functionally similar to today's VeryLongClassName approach.

> But having a single namespace, whose contents live in completely diverged places in the tree, seems quite bad to me.

If we do share a namespace across //chrome/browser/<foo> and //components/<foo>, then I expect this to be the same feature, where //components/<foo> is the x-platform impl, and //chrome/browser/<foo> is the platform-specific glue. Using the same namespace is desired.

> What about other code? Simply removing the namespace guidance entirely means that the standing rule is the google3 rule, which no code in Chromium obeys, or likely ever wants to obey. So I think that's out. How do you want to codify the updated guidance such that it covers everywhere?

I don't think we should. 

[Erik Chen]

Hi Peter,

I don't think we're on the same page here. I've answered your questions below for completeness. If you think it's important I'm happy to take a VC.

To summarize:

* From my perspective, being able to use namespaces is a helpful organization tool. Alongside some of the other design principles, it's a forcing function that causes authors to write modular features, with well defined API boundaries. This will begin to solve some of the larger architectural issues with //chrome.

* Based on your questions and comments above, it seems like you're interested in a machine-automatable, not-too-verbose, conflict-free mechanism for defining namespaces. Since no one has been able to come up with a system to do so, the default guidance until now is to not use namespaces.

> TLDR: Can you clarify the problems with the status quo, and how this would be a clear improvement? Changes here might be challenging.

The broader problem is that "features" in //chrome are not well defined, and do not have clear API surfaces. They often involve scattering functions/classes across a variety of files. Implementation details for multiple features are often mingled together. This results in code that is hard to test, spooky action at a distance, unexpected re-entrancy, and other problems.

> Can these features live in components/? If they are only used once, then components/README.md pretty clearly says "no". But maybe some of them would be used from multiple places (e.g. different UI implementations)? If so, they would be candidates to move there.

This is not the right question to ask. The problem today is we don't have clearly delineated features with well defined APIs/dependencies. If we had features, whether the features live in //components or //chrome is a smaller concern we can address later.

> If not: how would we choose the namespace names?

Use best judgement. It's not the name that matters, it's the conceptual organization of the code. Most of the concerns about nomenclature seem to be about verbosity. This is a symptom of the underlying problem: lack of clear (and limited) API surfaces. If a feature only has 2 public APIs, it frankly doesn't matter whether the namespace is 5 or 20 characters. Code inside the feature will use a namespace <foo> {} block, and code outside the feature will use foo::PublicAPI(), which is functionally similar to today's VeryLongClassName approach.

> But having a single namespace, whose contents live in completely diverged places in the tree, seems quite bad to me.

If we do share a namespace across //chrome/browser/<foo> and //components/<foo>, then I expect this to be the same feature, where //components/<foo> is the x-platform impl, and //chrome/browser/<foo> is the platform-specific glue. Using the same namespace is desired.

> What about other code? Simply removing the namespace guidance entirely means that the standing rule is the google3 rule, which no code in Chromium obeys, or likely ever wants to obey. So I think that's out. How do you want to codify the updated guidance such that it covers everywhere?

I don't think we should. 

[Peter Kasting]

On Friday, May 10, 2024 at 7:20:48 AM UTC-7 Jean-Philippe Gravel wrote:

The current Google3 guidance (https://abseil.io/tips/130) actually recommends against namespace nesting and against naming namespaces based on folder paths. 

Thanks for this! I had re-skimmed the "namespace names" section of the Google Style Guide last night, but somehow didn't absorb its link to this or the impact of the recommendations here.

This guidance makes sense to me (and, if we complied, would alleviate a number of clashes I've seen in my local jumbo build experiments). And it would allow for more namespacing, which I think would be nice.

What we don't have (that it recommends, and I think we need) is a database of namespaces used in the project, so we can avoid clashes (and people can figure out what corresponds to what). Is there existing best practice somewhere we can use? Should we just have a text file somewhere with a big list?

Finally, Google's guidance recommends one namespace "per project" if possible. Is there a rough guide we can give people on what a "project" would be in Chrome? As Dana notes, we'd likely want namespaces to not exceed module boundaries (in my head, module boundaries would usually correspond 1:1 with namespaces). I don't know what that implies precisely, but maybe it means c/b/foobar/ and components/foobar/, which are meant to be consumed by different code and different platforms, should not both be foobar::?

PK

[Peter Kasting]

On Friday, May 10, 2024 at 7:29:52 AM UTC-7 Erik Chen wrote:

I'm happy to take a VC.

VCs are great for getting us in sync, but the arbiter is "rough consensus of the list" rather than me :).

* Based on your questions and comments above, it seems like you're interested in a machine-automatable, not-too-verbose, conflict-free mechanism for defining namespaces. Since no one has been able to come up with a system to do so, the default guidance until now is to not use namespaces.

For clarity, I'm mostly trying to summarize the past input and decisions, which included a lot of comments from blundell@, jam@, brettw@, darin@, etc. However, we haven't re-discussed our policies since Google stopped recommending hierarchical, path-based namespaces (which, somehow, I missed that they did!). That's a sea change that IMO warrants a revisit.

Re: the points above -- I don't think machine-automatable is necessary, but not-too-verbose and conflict-free would be good. I think you're getting "machine-automatable" from the idea (espoused by many different folks in the past) that there should be some predictable mapping between file path and namespace, or else things felt random and confusing. I sympathize with that, but I don't know that it's possible, and the perfect shouldn't be the enemy of the good. "Global database" is sufficient for me personally.

If we do share a namespace across //chrome/browser/<foo> and //components/<foo>, then I expect this to be the same feature, where //components/<foo> is the x-platform impl, and //chrome/browser/<foo> is the platform-specific glue. Using the same namespace is desired.

As mentioned in my reply to jpgravel@, I worry about the consequences of that in a modules world. It might make sense to say that for a namespace foobar:: corresponding to some directory a/b/c, all the code in the directory tree rooted there is in foobar::, and none of the code outside it is. (This doesn't request hierarchical namespace or require namespace<->pathname correspondence; it just attempts to avoid directory and namespace divergence.)

PK

[danakj]

On Fri, May 10, 2024 at 10:45 AM Peter Kasting <pkas...@chromium.org> wrote:

On Friday, May 10, 2024 at 7:20:48 AM UTC-7 Jean-Philippe Gravel wrote:

The current Google3 guidance (https://abseil.io/tips/130) actually recommends against namespace nesting and against naming namespaces based on folder paths. 

Thanks for this! I had re-skimmed the "namespace names" section of the Google Style Guide last night, but somehow didn't absorb its link to this or the impact of the recommendations here.

This guidance makes sense to me (and, if we complied, would alleviate a number of clashes I've seen in my local jumbo build experiments). And it would allow for more namespacing, which I think would be nice.

What we don't have (that it recommends, and I think we need) is a database of namespaces used in the project, so we can avoid clashes (and people can figure out what corresponds to what). Is there existing best practice somewhere we can use? Should we just have a text file somewhere with a big list?

I can very happily see a //NAMESPACES file that has a list of namespaces and the source tree root where they are found.

Since some namespaces occur in multiple places (whether this is a bug or not), they'd appear in the list more than once.

[danakj]

I think we should rewrite that guidance to say that we don't use a `chrome` namespace for all of //chrome, we put the core of it in the global namespace, with some wordsmithing applied.

On Fri, May 10, 2024 at 4:20 PM Erik Chen <erik...@chromium.org> wrote:

If the folks on this list want to put together global guidance for the chromium repository on how to deal with namespaces, please do so!

Right now there is guidance under styleguide/c++ that prohibits namespaces, but only in //chrome. Given that //chrome/OWNERS want to allow namespaces, and there are already plenty of examples of namespaces in //chrome, can we remove this guidance from styleguide/c++?

[Erik Chen]

If the folks on this list want to put together global guidance for the chromium repository on how to deal with namespaces, please do so!

Right now there is guidance under styleguide/c++ that prohibits namespaces, but only in //chrome. Given that //chrome/OWNERS want to allow namespaces, and there are already plenty of examples of namespaces in //chrome, can we remove this guidance from styleguide/c++?

On Saturday, May 11, 2024 at 12:09:59 AM UTC+9 danakj wrote:

[Peter Kasting]

On Friday, May 10, 2024 at 1:38:21 PM UTC-7 danakj wrote:

I think we should rewrite that guidance to say that we don't use a `chrome` namespace for all of //chrome, we put the core of it in the global namespace, with some wordsmithing applied.

That seems necessary but not sufficient, but maybe further clarity is "nice to have but shouldn't block this request"? Feedback welcome. (I largely want to get out of //chrome OWNERS' way, but I don't think we've internalized the current Google guidance, and if we're going to change our guidance anyway, we might as well be clear and consistent immediately.)

Proposal: "Most code should be in a namespace, with the exception of code under //chrome, which may be in the global namespace (do not use the `chrome::` namespace). Record namespace use in //NAMESPACES, to avoid overlap. Minimize use of nested namespaces, as they do not actually improve encapsulation; if a nested namespace is needed, do not reuse the name of any top-level namespace (see https://abseil.io/tips/130). If some code in a directory is in a namespace, then if possible, place the whole subtree in that namespace, and don't use that namespace for code elsewhere (e.g. don't share a namespace between //components/x and //content/x)."

This would necessitate actually creating a //NAMESPACES file, which I can do if someone else doesn't want to.

Rationale: All but the last sentence tries to summarize upstream style, with the "chrome::" carveout specific to us. The last bit aims to avoid future modules issues and hopefully balance "reasonable namespace<->path mapping" with "light touch, let OWNERS decide". Maybe others dislike, though.

PK

[Peter Kasting]

There's been radio silence here. Any opinions on my proposal (and, if either you dislike it or think it will take time to implement, any opinions on landing Dana's proposed rewording more quickly)?

PK

[Erik Chen]

I think we can and should share namespaces when a single feature has code in multiple directories (e.g. //components/foo, //chrome/browser/foo). I am generally less worried about namespace collision and think that //NAMESPACES adds overhead for little gain, but feel less strongly about that. If possible, I'd prefer to land Dana's proposed rewording now. 

[Peter Kasting]

The primary reason not to do that was modules and Rust. Do you have reason to believe this won't be problematic for either of those?

PK

[Jean-Philippe Gravel]

> Most code should be in a namespace, with the exception of code under //chrome, which may be in the global namespace (do not use the `chrome::` namespace).

Is this to accommodate existing code, or is this really the ideal we want to promote going forward? Global symbols are dangerous because they can conflict with symbols having the same name in any namespace anywhere in the code base. I just opened a file at random and found chrome/utility/importer/importer.h which defines the class `::Importer`. This is an extremely generic name which can conflict with other symbols of the same name anywhere in the code. It could even conflict with other global symbols in third_party libraries, in violation of the One Definition Rule. Surely we do not want to encourage any more symbols like this?

> if a nested namespace is needed, do not reuse the name of any top-level namespace

Simply not reusing existing top-level namespace might not be sufficient. The TOTW says: "Try to give sub-namespaces unique names that are unlikely to collide with future top-levels." That is, we should choose names that are unique right now, and that are specific enough to not conflict with new namespaces created in the future. The TOTW also calls out: "No sub-namespaces for absl, testing, util, etc." These namespace names are bad because they are very likely to create conflicts. You could easily end up with a situation like: `::util`, `::foo::util`, `::foo::bar::util`. Should we include these in our recommendations?

On Thu, May 16, 2024 at 1:49 AM Erik Chen <erik...@chromium.org> wrote:

I'm not an expert on either. Do you have a link to previous discussions on the problems with namespace collision/shared namespaces and modules/Rust?

[Erik Chen]

I'm not an expert on either. Do you have a link to previous discussions on the problems with namespace collision/shared namespaces and modules/Rust?

[Peter Kasting]

On Thursday, May 16, 2024 at 5:51:16 AM UTC-7 Jean-Philippe Gravel wrote:

> Most code should be in a namespace, with the exception of code under //chrome, which may be in the global namespace (do not use the `chrome::` namespace).

Is this to accommodate existing code, or is this really the ideal we want to promote going forward? Global symbols are dangerous because they can conflict with symbols having the same name in any namespace anywhere in the code base.

Only if the code in that namespace has pulled in the code declaring the global symbol. The reason (many years ago) we decided chrome:: was not helpful was that chrome/ is a leaf node compared to the code in other directories that defines namespaces; chrome/ may rely on content/, for example, but content/ should never rely on chrome/. Therefore, it's not a problem if chrome/ globally-defines a symbol that content/ namespace-defines.

The problem with allowing namespaces in chrome/ code is that that undermines this assumption. If, say, chrome/browser/foo wants to define foo::Importer, it's very plausible that part of foo:: will pull in the declaration of ::Importer, and at that point you have to start explicitly qualifying everything and it gets ugly.

So there isn't a free lunch here. We allowed chrome/ to avoid using chrome:: because it wasn't using anything else either. Now the request is to let it define other namespaces but still put things in the global namespace. This is risky. But the alternatively are unappealing: mass-placing all existing chrome/ code in chrome::, or trying to audit all the existing chrome/ code, determine the correct spanning set of namespaces, and then assign things to those, before allowing any further namespace creation.

I'm not sure how to balance these trade-offs. Both Erik/Dana's proposal and my more detailed proposal come down on "well, just hope for the best", but that's why I want input on whether we think we can trust that.

When people get frustrated that cxx@ won't just let OWNERS do what they want with their subdirs, it may be because the ramifications of seemingly obvious, sane policies are often subtle and long-term.

 

> if a nested namespace is needed, do not reuse the name of any top-level namespace

Simply not reusing existing top-level namespace might not be sufficient. The TOTW says: "Try to give sub-namespaces unique names that are unlikely to collide with future top-levels." That is, we should choose names that are unique right now, and that are specific enough to not conflict with new namespaces created in the future. The TOTW also calls out: "No sub-namespaces for absl, testing, util, etc." These namespace names are bad because they are very likely to create conflicts. You could easily end up with a situation like: `::util`, `::foo::util`, `::foo::bar::util`. Should we include these in our recommendations?

Not sure. In the spirit of "every rule should carry its weight", if we're going to mention anything more, the "no absl, testing, util" make more sense to me than the other. I'm inclined to omit both and link to the ToTW with a bit about "more detail at"? Do you think that's sufficient?

PK

[Peter Kasting]

On Thursday, May 16, 2024 at 7:04:05 AM UTC-7 Erik Chen wrote:

I'm not an expert on either. Do you have a link to previous discussions on the problems with namespace collision/shared namespaces and modules/Rust?

It was Dana's comment earlier in this thread: https://groups.google.com/a/chromium.org/g/cxx/c/6B35ua7S4C8/m/lnWANLkkAgAJ

FWIW, I've encountered code in the omnibox and autofill (IIRC) using the same namespace for both chrome/ and components/, and it was surprisingly confusing. I was always looking in the wrong place for method definitions and mixing up where the dependency layers were. Having namespaces reflect layering concerns, and not just general "which feature is this in", would have been enormously helpful to me at these times.

PK 

[danakj]

Well, FWIW I guess in 1p Rust our crate names are going to be written as the full gn path, so the tail name can collide without an issue. And we won't use `*` imports which is what importing C++ modules effectively gives. So this is really a C++ problem. I don't know how much energy is worth spending on it, honestly. We're probably a decade from modules mattering in this way in our codebase and who knows  what else will be different by then.

[Peter Kasting]

If you don't think it will be an issue in Rust, I'm willing to drop it from the requirements and let people use their judgement. I think modules are less than a decade away (I'd bet 4 years), but we will have much larger and more difficult problems than this to solve if we want to modularize our code and not just `import std.compat;`.

PK

[Jean-Philippe Gravel]

> I'm inclined to omit both and link to the ToTW with a bit about "more detail at"? Do you think that's sufficient?

Sounds good. I suppose we should focus on explaining how Chromium's style guide differs from Google's instead of rephrasing what is already written.

[Peter Kasting]

Based on Erik's and Dana's feedback, I'm revising my proposal to:

"Most code should be in a namespace, with the exception of code under //chrome, which may be in the global namespace (do not use the `chrome::` namespace). Record namespace use in //NAMESPACES, to avoid overlap. Minimize use of nested namespaces, as they do not actually improve encapsulation; if a nested namespace is needed, do not reuse the name of any top-level namespace. For more detailed guidance and rationale, see https://abseil.io/tips/130."

I worry that we're going to run into more namespace-ambiguity issues inside //chrome with this over time, but I am not sure there's a great path forward.

Given the continued radio silence here, I'm calling the question. Please reply with your thoughts within the next week; given that the above is almost directly Google style, continued silence will be considered assent.

PK

[Adam Rice]

Maybe explicitly mention that nested "internal" namespaces are commonly used for implementation details and are okay to use when it is not possible to keep the implementation details out of the header file?

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAd_jnfkyxZ_3fk5POiKyaAsKBX-0FeRZQfAFgg4%2BFV0A%40mail.gmail.com.

[Greg Thompson]

I'm concerned about the manual step of consulting/editing a //NAMESPACES file. It seems like one more thing that reviewers need to add to their list of things to check. Unless, of course, it's automated via presubmit/tricium/somesuch. Is that practical?

For the issue of well-defined boundaries, I think good structure in the GN files and prolific use of `public` rather than lumping everything in `sources` is even better than namespaces.

I'm not sure that it should matter since it's just an opinion, but here's mine: I have repeatedly thought "how is this not just noise?" when browsing code in //chrome that uses namespaces.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAC_ixdxm4uNqyGYBSF3VVUxPA38utzpkkys%2BpDiZXibxX2%3D_Hg%40mail.gmail.com.

[Harald Alvestrand]

If doing a //NAMESPACES file, consulting this should be a presubmit check, not a "you have to manually look there". So it should never need to be looked at by reviewers.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKQnr0QO%2BZ4oEC62XTn37ffTr4AreySxs-%3D8M8Here5brXfxUw%40mail.gmail.com.

[Erik Chen]

I don't understand the mechanism or rationale for "Record namespace use in //NAMESPACES, to avoid overlap"

Is this going to be an ordered list? If I want to use "namespace foo" and "foo" is already in //NAMESPACES, does that mean I can use it because it's already in use, or that I shouldn't use it because it's already in use? Does this include namespaces from third-party libraries? Is there precedence for trying to enumerate all namespaces?

I asked this earlier but maybe I missed the answer: what is the problem with namespace collision?  The earlier response was "The primary reason not to do that was modules and Rust. Do you have reason to believe this won't be problematic for either of those?" I think we determined Rust was not an issue, and that C++ modules were several years out, but I still don't understand why C++ modules have issues with namespace collision.

[Peter Kasting]

On Tuesday, May 21, 2024 at 8:31:37 PM UTC-7 Adam Rice wrote:

Maybe explicitly mention that nested "internal" namespaces are commonly used for implementation details and are okay to use when it is not possible to keep the implementation details out of the header file?

The Google Style Guide already says this in https://google.github.io/styleguide/cppguide.html#Namespaces, so we normally wouldn't reiterate it.

PK

[Peter Kasting]

On Wednesday, May 22, 2024 at 3:15:57 AM UTC-7 g...@chromium.org wrote:

I'm concerned about the manual step of consulting/editing a //NAMESPACES file. It seems like one more thing that reviewers need to add to their list of things to check. Unless, of course, it's automated via presubmit/tricium/somesuch. Is that practical?

Should be easy to PRESUBMIT this. PRESUBMIT uses regexes, so something like `^namespace (?:\w+::)*(\w+) {` should capture the leaf namespace name, and then the script can simply look it up in //NAMESPACES and warn (not error!) if there's no match. It's appropriate for reviewers to consider a higher-level question like "is this appropriate use of namespaces at all", but checking compliance with this bookkeeping should never be something people waste time on.

For the issue of well-defined boundaries, I think good structure in the GN files and prolific use of `public` rather than lumping everything in `sources` is even better than namespaces.

I'm not sure that it should matter since it's just an opinion, but here's mine: I have repeatedly thought "how is this not just noise?" when browsing code in //chrome that uses namespaces.

IMO both build dependencies/visibility and namespaces can serve useful purposes.

The GN stuff helps ensure that symbols don't leak out of libraries. Namespaces in code primarily help the reader understand what conceptual group the code is in, and secondarily prevent symbol clashes within the same library.

I think Erik & co. want to improve conceptual organization. Like you, I'm not convinced this is going to be a big benefit, but I don't think we should forbid things the Google Style Guide allows just because we don't think they're helpful. My main concern here is simply "since so much code in //chrome _doesn't_ use a namespace, adding namespaces to some of it could make symbol conflicts worse." But since no one else has spoken to that, I think it's a concern that's not widely shared.

PK

[Peter Kasting]

On Wednesday, May 22, 2024 at 6:58:10 AM UTC-7 Erik Chen wrote:

I don't understand the mechanism or rationale for "Record namespace use in //NAMESPACES, to avoid overlap"

Is this going to be an ordered list?

Yes.

 

If I want to use "namespace foo" and "foo" is already in //NAMESPACES, does that mean I can use it because it's already in use, or that I shouldn't use it because it's already in use?

If the "foo" is for an unrelated purpose, then you can't use it, unless you rename existing usage.

 

Does this include namespaces from third-party libraries?

No.

 

Is there precedence for trying to enumerate all namespaces?

You mean, has someone already done something like that on our code? I don't believe so. Should be easy to collect with a script. My intent was not to put in language asking people to use //NAMESPACES until it was fleshed out, presumably by me, because I don't foresee others volunteering.

I asked this earlier but maybe I missed the answer: what is the problem with namespace collision?  The earlier response was "The primary reason not to do that was modules and Rust. Do you have reason to believe this won't be problematic for either of those?"

That was the answer on "why not share a namespace across directories, e.g. //components and //chrome". Namespace collision issues are unrelated to modules or Rust. https://abseil.io/tips/130 covers them in depth, but the short answer is that lookup stops as soon as it finds a matching namespace name, even if that name doesn't contain the requested symbol. Thus all code in a nested namespace can no longer see any code in a higher-level namespace of the same name (even if it's not an ancestor!) unless it starts fully-qualifying all of those symbols.

Hypothetical example:

• //components/colors is created to handle cross-project color-handling, using namespace `colors`.
• //chrome/browser/ui/customizer is created using namespace `customizer`.
• The customizer grows in scope, encompassing images, fonts, colors, etc., so it's split into subdirs using nested namespaces. One is //chrome/browser/ui/customizer/colors, using namespace `customizer::colors`.
• Suddenly code in that subdir cannot find any symbols from //components/colors, even though they were qualified with `colors::`. The compile breaks are mysterious (though easily fixable by changing `colors::` to `::colors::`).

It turns out this is already not-hypothetical today. Multiple times I've had extremely confusing compile failures due to this precise problem in deeply-nested namespace hierarchies (where you try to access some top-level symbol in foobar::, not realizing that there's also a foobar:: ancestor of your current namespace). This was more likely with jumbo enabled (and will be if we ever re-introduced it), but it has happened to me even without it.

PK

[Sylvain Defresne]

How should //NAMESPACES work with regards to //chrome vs //ios/chrome? Can we reuse the same namespace across both? Should they be exclusive (i.e. should namespace in //ios/chrome use an ios_ prefix)?

With regards to sharing namespace between //components and //chrome and //ios/chrome, I know that lots of engineers are currently choosing to reuse the same namespace for their feature in //components/$feature and the embedder specific part in //ios/chrome/browser/$feature. If I understand the proposal correctly, this is not something that should be done. Are there plans to change //ios/chrome and //chrome to remove the existing usage?

-- Sylvain

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/05a69193-5dc7-42fc-b85d-156a488d5561n%40chromium.org.

[Peter Kasting]

On Wed, May 22, 2024, 8:28 AM Sylvain Defresne <sdef...@chromium.org> wrote:

How should //NAMESPACES work with regards to //chrome vs //ios/chrome? Can we reuse the same namespace across both?

Yes.

Should they be exclusive (i.e. should namespace in //ios/chrome use an ios_ prefix)?

Up to you.

With regards to sharing namespace between //components and //chrome and //ios/chrome, I know that lots of engineers are currently choosing to reuse the same namespace for their feature in //components/$feature and the embedder specific part in //ios/chrome/browser/$feature. If I understand the proposal correctly, this is not something that should be done.

In my first proposal, it was frowned on, but the second version drops the guidance against doing that, so there's no problem here.

PK

[Erik Chen]

Thanks Peter for the link. It seems like the concern is not about namespace collision, but nested namespace collision. For that reason the google style guide recommends against namespace nesting. I don't think we need to restate this in our derived style guide.

If I want to use "namespace foo" and "foo" is already in //NAMESPACES, does that mean I can use it because it's already in use, or that I shouldn't use it because it's already in use?

> If the "foo" is for an unrelated purpose, then you can't use it, unless you rename existing usage.

I think you missed my question. How does the developer tell if "foo" is for an unrelated purpose, other than manually auditing all code that uses "namespace foo", which seems like a big ask?

I think we should simplify to:

"Most code should be in a namespace, with the exception of code under //chrome, which may be in the global namespace (do not use the `chrome::` namespace)."

[Peter Kasting]

On Wednesday, May 22, 2024 at 2:03:40 PM UTC-7 Erik Chen wrote:

Thanks Peter for the link. It seems like the concern is not about namespace collision, but nested namespace collision. For that reason the google style guide recommends against namespace nesting. I don't think we need to restate this in our derived style guide.

The section you link to doesn't recommend against nesting (though it does recommend against "overly deep" nesting). That more stringent recommendation is only in the ToTW, so it wouldn't necessarily apply to us unless we restated it. (Also, given the pervasiveness of violations of this in our codebase, clearly I'm not the only one who's been mistaken about the style guidance for years, so explicitly restating it seems good anyway.)

Top-level namespace collision is more obviously bad, so there's less need to explicitly address it. It seems clear to me, though, that if completely unrelated subprojects use the same top-level namespace, things are ripe for both misunderstanding and symbol conflicts, and we want to avoid that.

Also keep in mind that this text is intended for the "Dos And Don'ts" page, which, while it should be cited in reviews and considered "best practice", does not carry quite the mandatory heft of the full style guide -- if author and reviewer agree to bypass it, so be it.

If I want to use "namespace foo" and "foo" is already in //NAMESPACES, does that mean I can use it because it's already in use, or that I shouldn't use it because it's already in use?

> If the "foo" is for an unrelated purpose, then you can't use it, unless you rename existing usage.

I think you missed my question. How does the developer tell if "foo" is for an unrelated purpose, other than manually auditing all code that uses "namespace foo", which seems like a big ask?

Because I intend to list the associated directories for each entry in //NAMESPACES, and have the presubmit check that new usage is in a descendant of one. (I did actually think all this through beforehand but assumed it would be clear when the file was constructed and that this list's concern would be whether we should comply with Google's recommendations at all, rather than wanting the details. Mea culpa.)

PK

[Erik Chen]

Going back to Peter's original proposal:

"Most code should be in a namespace, with the exception of code under //chrome, which may be in the global namespace (do not use the `chrome::` namespace). Record namespace use in //NAMESPACES, to avoid overlap. Minimize use of nested namespaces, as they do not actually improve encapsulation; if a nested namespace is needed, do not reuse the name of any top-level namespace. For more detailed guidance and rationale, see https://abseil.io/tips/130."

The first and 3rd parts LGTM. I don't think //NAMESPACES will provide much utility (unless we also target 3rd-party libraries, which opens its own can of worms). That being said if it's automated I also don't think it will add much cost. I'll defer to people who have stronger opinions.

[John Admanski]

For what it's worth I think the point of the whole "you should have a database" recommendation is that namespaces should be "owned" in the sense that for any given top-level namespace you should have some person/group/project which is responsible for deciding what gets to go into it. The database then just follows from that in the sense that it's probably a good idea to have a straightforward way to track down if a name is in use or not, and which persons are responsible for it.

This doesn't necessarily require something particularly strict, Google's internal system isn't even enforced and basically just runs on the expectation that everyone is trying to operate in good faith, as far as I can tell. Of course, the //chrome codebase is more coherent than Google's entire internal repository, and obviously has a different contribution structure, so it's not one size fits all, but I think that the ToTW best practice isn't intended to be all that specific in its recommendation.

(I'm not trying to argue for or against //NAMESPACES here, I just thought this might be some helpful context?)

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAEYHnr2S8u3PoAZt4jbxweg04OAf9%2BU8KfB4bveb%3DfntinA3oA%40mail.gmail.com.

[Łukasz Anforowicz]

On Thu, May 23, 2024 at 2:05 PM John Admanski <jadm...@chromium.org> wrote:

For what it's worth I think the point of the whole "you should have a database" recommendation is that namespaces should be "owned" in the sense that for any given top-level namespace you should have some person/group/project which is responsible for deciding what gets to go into it. The database then just follows from that in the sense that it's probably a good idea to have a straightforward way to track down if a name is in use or not, and which persons are responsible for it.

This doesn't necessarily require something particularly strict, Google's internal system isn't even enforced and basically just runs on the expectation that everyone is trying to operate in good faith, as far as I can tell.

One rule that I've seen applied to Google's internal system is to check who owns the internal shortlink at go/<namespace name> - if the team owns it then it gets to use the C++ namespace with the same name.  (At least this is what I remember being told for cl/502682654 and having to work on taking over ownership of the go/rstd short link before considering using this namespace.  Although in cl/510522956 we later settled on go/rs_std which was unclaimed...)

 

(I'm not trying to argue for or against //NAMESPACES here, I just thought this might be some helpful context?)

On Wed, May 22, 2024 at 9:38 PM Erik Chen <erik...@chromium.org> wrote:

Going back to Peter's original proposal:
"Most code should be in a namespace, with the exception of code under //chrome, which may be in the global namespace (do not use the `chrome::` namespace). Record namespace use in //NAMESPACES, to avoid overlap. Minimize use of nested namespaces, as they do not actually improve encapsulation; if a nested namespace is needed, do not reuse the name of any top-level namespace. For more detailed guidance and rationale, see https://abseil.io/tips/130."

The first and 3rd parts LGTM. I don't think //NAMESPACES will provide much utility (unless we also target 3rd-party libraries, which opens its own can of worms). That being said if it's automated I also don't think it will add much cost. I'll defer to people who have stronger opinions.

On Thu, May 23, 2024 at 12:57 PM Peter Kasting <pkas...@chromium.org> wrote:

On Wednesday, May 22, 2024 at 2:03:40 PM UTC-7 Erik Chen wrote:

Thanks Peter for the link. It seems like the concern is not about namespace collision, but nested namespace collision. For that reason the google style guide recommends against namespace nesting. I don't think we need to restate this in our derived style guide.

The section you link to doesn't recommend against nesting (though it does recommend against "overly deep" nesting). That more stringent recommendation is only in the ToTW, so it wouldn't necessarily apply to us unless we restated it. (Also, given the pervasiveness of violations of this in our codebase, clearly I'm not the only one who's been mistaken about the style guidance for years, so explicitly restating it seems good anyway.)

Top-level namespace collision is more obviously bad, so there's less need to explicitly address it. It seems clear to me, though, that if completely unrelated subprojects use the same top-level namespace, things are ripe for both misunderstanding and symbol conflicts, and we want to avoid that.

Also keep in mind that this text is intended for the "Dos And Don'ts" page, which, while it should be cited in reviews and considered "best practice", does not carry quite the mandatory heft of the full style guide -- if author and reviewer agree to bypass it, so be it.

If I want to use "namespace foo" and "foo" is already in //NAMESPACES, does that mean I can use it because it's already in use, or that I shouldn't use it because it's already in use?

> If the "foo" is for an unrelated purpose, then you can't use it, unless you rename existing usage.

I think you missed my question. How does the developer tell if "foo" is for an unrelated purpose, other than manually auditing all code that uses "namespace foo", which seems like a big ask?

Because I intend to list the associated directories for each entry in //NAMESPACES, and have the presubmit check that new usage is in a descendant of one. (I did actually think all this through beforehand but assumed it would be clear when the file was constructed and that this list's concern would be whether we should comply with Google's recommendations at all, rather than wanting the details. Mea culpa.)

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAEYHnr2S8u3PoAZt4jbxweg04OAf9%2BU8KfB4bveb%3DfntinA3oA%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAAE6ASDPy2_o44W0MZ3S3voq622Qjr2vSD_VqFh0-Dr2xpwGA%40mail.gmail.com.

[Peter Kasting]

What I'm hearing sounds like general acceptance of the proposal direction, with a sense that there may be lower-overhead "good enough" ways to avoid namespace conflicts.

In turn this sounds like the right path forward is that we immediately land the wording change without the sentence on //NAMESPACES, I eventually attempt to write a patch to add such a file + appropriate PRESUBMIT checks to see how feasible it is, and if the PRESUBMIT owners are OK with that patch, we go ahead and add it and the relevant language at that point.

Any objections?

PK

[K. Moon]

Sounds good to me. I was going to comment earlier, but the thread generally resolved any points I would have made.

I do think keeping the "database" in the repository sounds like a good idea overall, whether or not it's enforced. (Enforcement would be nice, of course, and seems like it wouldn't be that challenging to have a 90% solution.)

A potentially simpler rule (in usage, not definition) that occurs to me now is that you can implicitly claim namespaces based on the directory path from the source root (perhaps with underscore separators if you need more than the first part), making certain adjustments for common top-level paths (like components/).

This wouldn't cover 100% of cases (so it'd be good to have the extra file as well), but seems to reflect common usage. (Let's call this the convention over configuration approach.)

Anyway, +1 to proceeding with your proposal.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/2971daab-d9b1-49ce-b40a-2ff6c294fb5an%40chromium.org.

[K. Moon]

Although on thinking about it more, one potential problem with NAMESPACES is that to comply with the style guidance, you need to ensure it's not used as a dictionary of allowed namespaces (which doesn't accomplish the goal of avoiding symbol conflicts), but that a given namespace is only used for the originally claimed purpose. At a minimum, this implies that it should also track "ownership" of the namespace.

Probably the best way to do that is to make sure each namespace entry is accompanied by a list of paths where it's allowed to be used in definitions (ignoring forward declarations somehow). Or maybe the list of paths would just be advisory.

(I suppose another approach might rely on per-directory metadata, and then merging it globally on check, but that seems technically more expensive.)

[K. Moon]

One last thought on implementing NAMESPACES: To avoid parsing C++, a simple algorithm could be:

1. Identify the top-level namespaces with a simple parser.

2. Find the namespace section with the most content.

3. The most contentful namespace must be one authorized by NAMESPACES.

4. Provide a bypass if the heuristic is wrong.

[Peter Kasting]

On Tuesday, May 28, 2024 at 8:40:45 AM UTC-7 Peter Kasting wrote:

sounds like the right path forward is that we immediately land the wording change without the sentence on //NAMESPACES, I eventually attempt to write a patch to add such a file + appropriate PRESUBMIT checks to see how feasible it is, and if the PRESUBMIT owners are OK with that patch, we go ahead and add it and the relevant language at that point.

I consider this approved at this point. Eric, you are welcome to make your CL that tweaks the style wording about this do the above and ping me for a +1.

PK