PSA: don't put anonymous namespaces in C++ header files.

[Erik Chen]

I've recently run across 2 different examples of recently written code that makes this mistake. A quick search shows 44 instances of this in //chrome, so there's probably more elsewhere. This anti-pattern is documented in the google C++ style guide. Violating this can cause ODR. Please avoid this.

[Peter Kasting]

Can we add a PRESUBMIT against `^namespace {` in header files? 

Reminder mails good, automated checks better :)

PK

On Wed, Jan 22, 2025, 1:57 PM Erik Chen <erik...@chromium.org> wrote:

I've recently run across 2 different examples of recently written code that makes this mistake. A quick search shows 44 instances of this in //chrome, so there's probably more elsewhere. This anti-pattern is documented in the google C++ style guide. Violating this can cause ODR. Please avoid this.

--
--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEYHnr1p6VTL44DzxQdR7bDT-3Fo9d-TGxBJdc08Nj4aLgx2Rg%40mail.gmail.com.

[Aaron Selya]

Wrote a CL that I think should address the issue https://chromium-review.googlesource.com/c/chromium/src/+/6192789

On Thursday, January 23, 2025 at 12:32:49 PM UTC-5 Peter Kasting wrote:

Can we add a PRESUBMIT against `^namespace {` in header files? 

Reminder mails good, automated checks better :)

PK

On Wed, Jan 22, 2025, 1:57 PM Erik Chen <erik...@chromium.org> wrote:

I've recently run across 2 different examples of recently written code that makes this mistake. A quick search shows 44 instances of this in //chrome, so there's probably more elsewhere. This anti-pattern is documented in the google C++ style guide. Violating this can cause ODR. Please avoid this.

--
--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Joe Mason]

Somewhat related: I recently found out that TEST definitions in gtest .cc files should NOT be in the anonymous namespace, because that foils some machinery to detect duplicate test names.

I find that hard to remember, because I've internalized the guideline that anything in a .cc file that's not explicitly exported should be in an anonymous namespace.

Is there a PRESUBMIT that could detect this? It seems more complicated than just looking for `^namespace {` because local helper functions and constants should still go in an anonymous namespace.

On Thu, Jan 23, 2025 at 3:14 PM Aaron Selya <se...@chromium.org> wrote:

Wrote a CL that I think should address the issue https://chromium-review.googlesource.com/c/chromium/src/+/6192789

On Thursday, January 23, 2025 at 12:32:49 PM UTC-5 Peter Kasting wrote:

Can we add a PRESUBMIT against `^namespace {` in header files? 

Reminder mails good, automated checks better :)

PK

On Wed, Jan 22, 2025, 1:57 PM Erik Chen <erik...@chromium.org> wrote:

I've recently run across 2 different examples of recently written code that makes this mistake. A quick search shows 44 instances of this in //chrome, so there's probably more elsewhere. This anti-pattern is documented in the google C++ style guide. Violating this can cause ODR. Please avoid this.

--
--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEYHnr1p6VTL44DzxQdR7bDT-3Fo9d-TGxBJdc08Nj4aLgx2Rg%40mail.gmail.com.

--

--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/48ec64c6-64f7-4391-827a-cb96543c2348n%40chromium.org.

[Alexei Svitkine]

Hmm, Google internal style guide docs actually disagree with the above:

go/c-readability-advice#anonymous-namespaces-in-tests

Is the limitation you mention something that's specific to Chromium's setup?

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAH%3DT95QFWgkSxsh4X%2ByMgfCbUcw3U4C23qPEn3%2BQfH9L4mAmNA%40mail.gmail.com.

[Peter Kasting]

On Mon, Jan 27, 2025 at 10:44 AM Alexei Svitkine <asvi...@chromium.org> wrote:

Hmm, Google internal style guide docs actually disagree with the above:

go/c-readability-advice#anonymous-namespaces-in-tests

Is the limitation you mention something that's specific to Chromium's setup?

Note that following Google's advice here prevents using FRIEND_TEST macros.

PK

[Erik Chen]

fwiw, I normally put gtest tests into an anonymous namespace, and expose relevant methods in the production code in a public block (sometimes with a comment saying //exposed for testing) rather than using FRIEND_TEST macros. This is purely my personal style and not something I care about standardizing across the codebase. I haven't seen issues with duplicate test names + gtest, I'd be curious to learn more.

--

--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFCfdexwhuzLnKXhzyiyRUg0e3Q3zTTBW7U6pcJb1gBKMw%40mail.gmail.com.

[K. Moon]

GoogleTest relies on symbol collisions to detect duplicate test names in the same binary. I think it's debatable how much this is a problem in practice, which is why it doesn't really come up, but I try not to define my tests in anonymous namespaces when consistent.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEYHnr1UQLzNAU-3mT83bSF95VvGjqGboFYh_N-YWyfe34Yi_w%40mail.gmail.com.

[Erik Chen]

oic. Duplicate test names is mostly a problem for our infrastructure, since we assume that unique name = unique test. I'm surprised that googletest relies on symbol collisions though, since it clearly knows the test names (for emitting results) so it could dump them into a set to check for non-collision. I agree that this is likely not a major problem in practice.

[K. Moon]

Link time vs. run time?

I'm not sure if the "don't use anonymous namespaces" recommendation even exists in the current GoogleTest documentation; I remember reading something like that guidance before, but can't find it now.

It does mention that friend tests require using the exact same namespaces as a reason not to use anonymous or inline namespaces, but that's a separate issue.

[Aaron Selya]

CL has landed that covers the header files. 

--

--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFCfdexwhuzLnKXhzyiyRUg0e3Q3zTTBW7U6pcJb1gBKMw%40mail.gmail.com.

[Adam Rice]

GoogleTest does detect duplicate test names at runtime.

In my view, tests should almost always be in an anonymous namespace. The exceptions would be if you need the FRIEND_TEST_ALL_PREFIXES() macro or "Value-Parameterized Abstract Tests" (https://google.github.io/googletest/advanced.html#creating-value-parameterized-abstract-tests).

I prefer narrowly-scoped ForTesting() methods to friending tests, and I've never used Value-Parameterized Abstract Tests, so my tests are always in an anonymous namespace.

Giving your tests global linkage just so you can detect collisions at link time instead of a second later when you run the tests seems like premature optimization to me.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALXbKU6iyMWFRW_Lzz6797HqTzXSB_0Egr_k4iKNp3wseeJRjQ%40mail.gmail.com.