Project

General

Profile

Task #2919

C++ style guidelines for namespace use

Added by Eric Irrgang 3 months ago. Updated 3 months ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
documentation
Target version:
-
Difficulty:
uncategorized
Close

Description

Use of C++ nested namespaces in GROMACS source has undocumented conventions that should be documented
  • to improve the code submission process, and
  • to provide a base-line for discussions on policy updates
One of more of the following documents ought to say something about currently disallowed, allowed, and encouraged (nested) namespaces, and the relationships to source code organization and symbol visibility. The other documents should probably be at most one "click" away.

To the extent they are known, reasons for the policy/convention should be noted or cited (though debate over the justification should be deferred to follow-up discussion).

Scope

  • This issue is intended to identify the GROMACS policy on namespace use as it has existed implicitly. Discussion of that policy should be deferred to follow-up issues.
  • The scope of this issue may be broadened to include scoping mechanisms, for completion, but should address specific C++ syntax (Purely lexical namespaces are described in dev-manual/naming.), which allows symbolic namespaces declared with namespace but also effectively through scoped enumerations, member types, and (static) class members.
  • Policy scope should be noted if it is conditional on the API layer at which it applies.

Additional references

The following may be useful to reference in discussion or examples for this or future issues.

History

#1 Updated by Roland Schulz 3 months ago

From ML:

GROMACS has a policy against using C++ `namespace` for logical structuring of code within the GROMACS package

I don't think that's true. We use nested namespaces test, detail, internal, analysismodules, and Nbnxm in multiple places. I think like anything else whenever it adds to clarity it should be encouraged. Also like anything else, if it is overused it might actually harms clarity (e.g. if you were to put each class into its own NS). I doubt it is easy to make general rule which say what's under-use and what overuse. It might be nice to have each module in its own NS. But that would be quite a bit refactoring. And we should first make sure to actually have everything in the gmx namespace at least.

When it comes to DumpInfo you couldn't do that as namespace. The reason is that registerModule in trajectoryanalysis/modules.cpp access it through the template argument. And you can't template by namespace but only by class. But even if it wasn't used in that way I think this is better done by a class than a namespace. The 3 pieces of information have a semantic connection. And thus a class is good to combine them. If they instead were 3 unrelated pieces of info needed in different parts of a module than globals inside a namespace would be more appropriate.

#2 Updated by Mark Abraham 3 months ago

Anonymous namespaces are and should be used for making clear that code is internal to the translation unit.

Code placed in headers for efficiency from inlining sometimes needs to be able to make clear that parts of it are internal details that should not be depended on, so we should have a convention to use a nested namespace like 'detail' or 'internal' for that (and to use only one such name).

I don't see any great value either way in referring to PmeLoadBalancer or pme::LoadBalancer. So I don't see why we'd put each module in its own namespace. If some other module would have a LoadBalancer then the namespace doesn't offer a clear value.

I agree with Roland that we should put everything in the gmx namespace before acting on any other decisions :-)

Modules should have internals that are not present in the interface to the module, but that is more effectively enforced by designating headers as internal to the module and checking that they are not used (with Doxygen, in our case). Putting code into pme::internal::SomeLoadBalancerDetail doesn't seem to add much. Either way, since module internals can't clash with the internals of other modules (as above) the nested namespace doesn't prevent relevant name collisions.

The Nbnxm namespace is deliberately transitional while that module grows a proper interface.

The test namespace doesn't seem very valuable either way, as again some other checker (Doxygen in our case) should find and prohibit invalid dependencies (e.g. production code depending on our floating-point tolerances used for testing).

Perhaps the use of the analysismodules namespace was motivated by the likelihood of names like select, angle, distance, etc. clashing, but that seems unlikely to occur in practice.

#3 Updated by Eric Irrgang 3 months ago

Roland Schulz wrote:

The 3 pieces of information have a semantic connection.

Ah! I had not noticed that this builds compatibility with that template, and definitely helps to clarify the tight coupling of the three values.

#4 Updated by Eric Irrgang 3 months ago

Mark Abraham wrote:

'detail' or 'internal' for that (and to use only one such name).

Is there currently a distinction between gmx::detail and gmx::internal? Both are currently used, but internal is used more.

Is there a consensus now about which is preferred or is the current distinction not prescriptive?

#5 Updated by Eric Irrgang 3 months ago

Roland Schulz wrote:

registerModule in trajectoryanalysis/modules.cpp access it through the template argument

To clarify the pattern in DumpInfo, I started https://gerrit.gromacs.org/c/gromacs/+/10483. DumpInfo::create does not look like it has the function signature required for ModuleInfo in its current state, but presumably it is migrating in that direction. But InsertMoleculesInfo uses accessors instead of data members. That may be related to planned updates to the framework, but I stopped short of proposing additional documentation for InsertMoleculesInfo, ReportMethodsInfo, and pdb2gmxInfo. With additional feedback, I can expand or abandon that change, as appropriate.

With the current feedback, I think I can draft a few words of clarification for the dev guide (separate patch, of course).

#6 Updated by Roland Schulz 3 months ago

Mark Abraham wrote:

Anonymous namespaces are and should be used for making clear that code is internal to the translation unit.

Code placed in headers for efficiency from inlining sometimes needs to be able to make clear that parts of it are internal details that should not be depended on, so we should have a convention to use a nested namespace like 'detail' or 'internal' for that (and to use only one such name).

I would go a bit further. I think as much as possible should be put into anonymous namespace (and should be preferred over static functions we still use in many files). For headers anonymous namespace never make sense. As much as possible (anything only used by the header and its corresponding source file) should go into the detail/internal namespace.

I think we use both names the same way. I think what happened was: Initially we used internal. Than we started to use detail because it is used more commonly in other projects (see e.g. https://cppdepend.com/blog/?p=336). I agree it would make sense to rename one so we only use one. My vote would be on detail. I like the consistency with other projects (given that we don't have too many usages of internal yet).

#7 Updated by Eric Irrgang 3 months ago

Roland Schulz wrote:

My vote would be on detail.

Mine, too. detail is pretty standard. internal requires more explanation. Especially with doxygen macros like \libinternal floating around, it looks like it has a GROMACS-specific meaning.

#8 Updated by Carsten Kutzner 3 months ago

+1

#9 Updated by Eric Irrgang 3 months ago

gtest uses ::testing::internal and we have a couple of using lines for gtest stuff, but GROMACS also use ::testing

We could take this occasion to be prescriptive about testing namespaces. Should we use one or not? Should it be consistently gmxtest, testing, or something else?

#10 Updated by Mark Abraham 3 months ago

Roland Schulz wrote:

Mark Abraham wrote:

Anonymous namespaces are and should be used for making clear that code is internal to the translation unit.

Code placed in headers for efficiency from inlining sometimes needs to be able to make clear that parts of it are internal details that should not be depended on, so we should have a convention to use a nested namespace like 'detail' or 'internal' for that (and to use only one such name).

I would go a bit further. I think as much as possible should be put into anonymous namespace (and should be preferred over static functions we still use in many files). For headers anonymous namespace never make sense. As much as possible (anything only used by the header and its corresponding source file) should go into the detail/internal namespace.

I think we use both names the same way. I think what happened was: Initially we used internal. Than we started to use detail because it is used more commonly in other projects (see e.g. https://cppdepend.com/blog/?p=336). I agree it would make sense to rename one so we only use one. My vote would be on detail. I like the consistency with other projects (given that we don't have too many usages of internal yet).

Sounds good.

Eric Irrgang wrote:

gtest uses ::testing::internal and we have a couple of using lines for gtest stuff, but GROMACS also use ::testing

::testing is from GoogleTest, we should leave it alone and avoid depending on its internals (though we do do it in one place for a decent reason). GROMACS should use things from ::testing (that's the point) and using ::testing::stuff is reasonable. GROMACS should not put anything into ::testing, because it's not our namespace - did you mean to suggest that we were, Eric?

We could take this occasion to be prescriptive about testing namespaces. Should we use one or not? Should it be consistently gmxtest, testing, or something else?

We have gmx::test, which helps keep our test code from clashing with gmx and from testing. We should not adopt ::testing because GoogleTest will feel free to put things into it.

#11 Updated by Eric Irrgang 3 months ago

Mark Abraham wrote:

::testing is from GoogleTest, we should leave it alone and avoid depending on its internals (though we do do it in one place for a decent reason). GROMACS should use things from ::testing (that's the point) and using ::testing::stuff is reasonable. GROMACS should not put anything into ::testing, because it's not our namespace - did you mean to suggest that we were, Eric?

Sorry, no, GROMACS puts stuff in ::gmx::testing, but I don't see it putting anything in ::testing

We have gmx::test, which helps keep our test code from clashing with gmx and from testing. We should not adopt ::testing because GoogleTest will feel free to put things into it.

We use both gmx::test and gmx::testing. Normalizing on ::gmx::test sounds good.

#12 Updated by Erik Lindahl 3 months ago

  • Tracker changed from Bug to Task
  • Affected version deleted (git master)

Also available in: Atom PDF