[CodingStandard] Rework anonymous namespace section to cover visibility more broadly

[jurahul]

• Rename anonymous namespace section and rework it to
  cover visibility more broadly.
• Add language suggesting restricting visibility as much as
  possible, using various C++ facilities.

[AaronBallman]

I think we should clarify https://llvm.org/docs/CodingStandards.html#anonymous-namespaces rather than add a new section specifically. WDYT?

[jurahul]

Sound good. This note is about *what* to internalize and the anonymous namespace is about *how* but I can move this line to the end of that section.

…

On Wed, Feb 12, 2025 at 5:25 AM Aaron Ballman ***@***.***> wrote: ***@***.**** commented on this pull request. I think we should clarify https://llvm.org/docs/CodingStandards.html#anonymous-namespaces rather than add a new section specifically. WDYT? — Reply to this email directly, view it on GitHub <#126775 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APRMUB7B7ZV5RDYDDE5BNFT2PNDVTAVCNFSM6AAAAABW5XAQIOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMJRHEYTKNRWGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jurahul]

Sound good. This note is about what to internalize and the anonymous namespace is about how but I can move this line to the end of that section.

Moved it to the end of the anonymous namespaces section. PTAL.

[AaronBallman]

I think this is really just clarifying the above paragraph. What if we said:

When you are looking at "``runHelper``" in the middle of a large C++ file,
you have no immediate way to tell if this function is local to the file.  In
contrast, when the function is marked static, you don't need to cross-reference
faraway places in the file to tell that the function is local. For this reason, non-
member functions and variables should be marked ``static`` rather than added
to an anonymous namespace.

[joker-eph]

I don't quite see this as achieving what we want here actually: this is just saying to prefer using static instead of anonymous, but it does not say anything about the policy of making thing private as much as possible.

What about adding it at the end of the intro like this (last paragraph is added):

After talking about namespaces in general, you may be wondering about anonymous
namespaces in particular. Anonymous namespaces are a great language feature
that tells the C++ compiler that the contents of the namespace are only visible
within the current translation unit, allowing more aggressive optimization and
eliminating the possibility of symbol name collisions. Anonymous namespaces are
to C++ as "static" is to C functions and global variables. While "static"
is available in C++, anonymous namespaces are more general: they can make entire
classes private to a file.
We aim to make private as much as possible every function or variable: if it is
defined in a .cpp file and need not be referenced outside that file, it should be
made private to this file.

That phrasing would cover also classes that can be put in anonymous namespaces as part of the intent to make things as private as possible.

[jurahul]

I agree with @joker-eph, again distinguishing between what needs to be internalized (last paragraph that @joker-eph suggested) vs how to achieve that (static for non-member functions and variables and anonymous namespaces for classes and structs). I'd reword the last paragraph a bit as follows, with some extra/redundant clarification:

We aim to make functions, variables, and classes as private as possible: If a non-member function or variable is defined in a .cpp file and need not be referenced outside that file, it should be made private to this file (by using static). Similarly, if a class or struct is defined in a .cpp file and need not be referenced outside that file, it should be made private to this file (by using anonymous namespace).

WDYT?

[jurahul]

Or to belabor on this more, may be this should be the first paragraph, saying that things should be as private as possible, and then going into how to achieve that via static or anon namespaces. The section can be renamed to something like privatize aggressively or keep it as is.

[AaronBallman]

Or to belabor on this more, may be this should be the first paragraph, saying that things should be as private as possible, and then going into how to achieve that via static or anon namespaces. The section can be renamed to something like privatize aggressively or keep it as is.

This format would make more sense to me. Basically, "Here's the principle we want to follow and why. Here's how to follow that principle." Should we also strongly encourage using private and protected access specifiers within classes too? Then the section is basically about all the ways in which you can restrict access to identifiers. (I don't know if I'm scope-creeping too much though.)

[jurahul]

Finally got around to updating this based on the suggestion above. @AaronBallman and @joker-eph PTAL.

[jurahul]

Sounds good. I’ll update the PR and rework this entire section.

…

On Fri, Feb 14, 2025 at 4:46 AM Aaron Ballman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In llvm/docs/CodingStandards.rst <#126775 (comment)>: > +Related to this issue, when non-member functions or variables are defined in a +`.cpp`` file and need not be referenced outside that file, make them `static`. Or to belabor on this more, may be this should be the first paragraph, saying that things should be as private as possible, and then going into how to achieve that via static or anon namespaces. The section can be renamed to something like privatize aggressively or keep it as is. This format would make more sense to me. Basically, "Here's the principle we want to follow and why. Here's how to follow that principle." Should we also strongly encourage using private and protected access specifiers within classes too? Then the section is basically about all the ways in which you can restrict access to identifiers. (I don't know if I'm scope-creeping too much though.) — Reply to this email directly, view it on GitHub <#126775 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APRMUB7HQU2TCRAGAN6ZS2D2PXQTTAVCNFSM6AAAAABW5XAQIOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMJXG4YDEOJZG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[AaronBallman]

LGTM with some minor nits

[jurahul]

Thanks, applied your suggestions.

[AaronBallman]

Thanks, applied your suggestions.

Thanks! Do you need us to land this for you?

[jurahul]

Thanks, applied your suggestions.

Thanks! Do you need us to land this for you?

No. I have commit access, will land once the checks are green again.

[shafik]

nit

[shafik]

the great here feels a bit like editorializing to me, while I don't disagree it kind of stands out wrt to the rest of the paragraph. I wouldn't bother changing it but for future reference.