isocpp / CppCoreGuidelines

The C++ Core Guidelines are a set of tried-and-true guidelines, rules, and best practices about coding in C++
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines
Other
42.55k stars 5.43k forks source link

Should SF.7 be less specific on how the namespace is modified, ban other mutations in .h files? #1658

Open ChrisGuzak opened 4 years ago

ChrisGuzak commented 4 years ago

SF.7: Don't write using namespace at global scope in a header file

the global namespace can be modified as follows

// foo.h
namespace foo {
    using namespace bar;
}

I've assume this rule intends to ban namespace mutations (other than straight additions), but but as written allows this. Should this be disallowed?

I suspect this was written to allow use of namespace aliases in functions scopes but missed this case.

oldnewthing commented 4 years ago

I interpreted the rule to mean more generally "Do not pollute shared namespaces (such as the global namespace)."

Importing names into private namespaces is fine (that's why they're private), but shared namespaces are not a good idea because you don't know what what the other people sharing the namespace will want.[1]

My proposal would be to rephrase as something like

Don't write using namespace in a header file to import names into a shared namespace (such as the global namespace)

// foo.h

using namespace bar; // bad idea
namespace foo::impl {
  using namespace bar; // okay, the impl namespace is private to the foo component
}

// unrelated.h
#include <foo.h>

namespace foo
{
   using namespace bar; // bad idea: Other clients of foo.h will find bar's types by mistake
}
void UnrelatedThing(foo::food food);

[1] It also creates easy opportunities for people to take dependencies upon what you thought was your own personal convenience.

// other.cpp
#include <foo.h>
#include <unrelated.h>

using namespace foo;

void Something(bargle bargle); // inadvertently relies on bargle being an alias for bar::bargle

You then update your unrelated.h component so it doesn't use bar any more, and you end up breaking other.cpp.

Or the authors of other.cpp realize that they aren't using the UnrelatedThing any more, so they remove the #include <unrelated.h> and their build breaks.

Or you do some cleanup like sorting header files alphabetically, only to find that baz.h had inadvertently relied upon foo.h having already imported all of bar into foo.

Cleaning up this mess can lead to chasing down a large cascade of dependencies. In frustration you may just give up and leave the bad code in place, because removing it is too much work.

// unrelated.h

#include <foo.h>

namespace foo
{
   using namespace bar; // we don't use bar, but many people rely on bar being imported into foo
}
void UnrelatedThing(foo::food food);
MikeGitb commented 4 years ago

There is also the question, if I'm using using namespace to import a 3rd party namespace to save some chars in my code (both reading and writing) or if I'm deliberately re-exporting the entities from one namespace as part of another (e.g. importing std::literals::chrono_literals; into std::chrono)

hsutter commented 4 years ago

Editors call: @gdr-at-ms and @ChrisGuzak have converged on a solution and @gdr-at-ms is assigned to implement it.

AraHaan commented 3 years ago

SF.7: Don't write using namespace at global scope in a header file

the global namespace can be modified as follows

// foo.h
namespace foo {
    using namespace bar;
}

I've assume this rule intends to ban namespace mutations (other than straight additions), but but as written allows this. Should this be disallowed?

I suspect this was written to allow use of namespace aliases in functions scopes but missed this case.

the issue is that the c++ standard library also uses this to inline things from their tr namespaces, I think for type-erase or something.

Or at least a few headers do it in the MSVC STL which can be seen here: https://github.com/Microsoft/STL/.