Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should SF.7 avoid to limit its applicability to when the directive is at global scope? #2208

Open
Aster89 opened this issue Jun 24, 2024 · 4 comments

Comments

@Aster89
Copy link

Aster89 commented Jun 24, 2024

Today I've spent quite some time to discover why adding something like

namespace foo::detail { /* stuff */ }

in a header of mine was causing havoc in a seemingly unrealted translation unit (unrelated to the one I was compiling when I made the change). The error was along these lines, where innocent-header.hpp is the only file I own:

error: 'FooBar' is not a member of 'foo::detail'; did you mean 'foo::bar::baz::detail::FooBar'?

The problem was a using directive:

  • when compiling my TU, everything was fine;
  • when compiling the other TU, my header ended up being included together with
    • a header that had a using directive,
    • and antoher header that made the mistake of leveraging that directive.

The failing translation unit was therefore looking like this:

// /* broken-header.hpp */
namespace foo {
namespace bar::baz::detail {
}
using namespace foo::bar::baz;
}

// /* innocent-header.hpp */
namespace foo::detail { /* stuff */ }

// /* thoughtless-header.hpp */
// #include "broken-header.hpp"
namespace foo::bar::baz {
namespace detail {
struct Foo {};
}
auto f() {
    return foo::detail::Foo{};
}
}

int main() {
    foo::f();
}

where the order of inclusion of broken-header.hpp and innocent-header.hpp is irrelevant.

I wouldn't say that thoughtless-header.hpp is innocent, but they could have made the mistake of writing foo::detailFooBar instead of foo::bar::baz::detail::FooBar just because they've distractedly Tab-completed with their IDE.

But I think that the real problem is still the using namepspace.

So why shouldn't we change the title of SF.7 the following?

Don’t write using namespace in a header file

@hsutter
Copy link
Contributor

hsutter commented Sep 26, 2024

Editors call: Thanks!

where innocent-header.hpp is the only file I own

We wonder if this is the problem, because all three files are adding things to a namespace and so normally would be expected to be owned by the same maintainer? And the author of broken-header.hpp presumably owns the namespace they are adding a using-directive into... can you elaborate on why innocent-header.hpp would be adding things to a namespace that is owned by the author of broken-header.hpp?

@Aster89
Copy link
Author

Aster89 commented Sep 27, 2024

Thanks for the reply, @hsutter.

It took a bit to get my head around the past, but I think I've reconstructed it, which also shows I've been a bit inaccurate when I posted the issue in June.

At that time, I contributed to that part of the codebase where broken-header.hpp lives.

So the answer to

why innocent-header.hpp would be adding things to a namespace that is owned by the author of broken-header.hpp?

is that I contributed to that sub-codebase and that namespace.

But I had already written the utility in a header in the part of the codebase I usually work with, innocent-header.hpp, which, crucially, also includes broken-header.hpp. Only when I finished developing it in mycomponent::, did I think "Oh, maybe it's worth sharing it in foo::". At that point I just replaced all of mycomponent:: in this new utility with foo::, but re-built my component once more (because why not?) before starting to cut-and-paste stuff. Boom.

And I have to say, luckily I tried to compile that in my codebase first, because otherwise the unit test I eventually wrote in the foo component for testing the utility that was made of namespace foo::detail { /* stuff */ }, does not #include "broken-header.hpp", so if I had written that utility in the foo component to start with, I would have most likely succeeded in writing the test and submitting, only to discover later, as a client of my own new utlity, that my component could not use it because including both innocent-header-but-in-that-foo-component.hpp and broken-header.hpp at the same time would trigger that compilation at the top of the original post.

But my point is that using namespace in header, even when not at global scope, can make it incompatible with another header, and that incompatibility would surface only when the two incompatible headers are included together, which could happen much later, when several clients of either one or the other header have already sprouted everywhere.


Yes, if I knew of that using namespace there would have been no issue.

But it's not really just "if I knew", it's "if I knew and remembered and had it ready in the L1 cache of my brain, if not a register". Plus, owners of codebases change with time.

@jwakely
Copy link
Contributor

jwakely commented Sep 28, 2024

"Don’t write using namespace in a header file" means I can't do this in a header file:

inline func(X& x) {
  using namespace std;
  // ...
  blah(x);
}

Now some people say preventing that is good (and I've been ridiculed on stackoverflow by such people) but the risk is that func stops compiling or changes meaning at some point in the future if std::blah gets added. There's no danger here of the code changing meaning depending on header order.

So maybe what you're asking for is for SF.7 to talk about namespace scope (i.e. outside of function bodies) not global scope. That would have avoided your problem, without banning the code above that uses it in a body of an inline function.

But I think the use of global scope is intentional, because it still allows importing all names from one namespace into another:

namespace mine {
  using namespace yours;
}

The broken-header.hpp case just seems like a bad use of using namespace (why bother having foo::bar::baz below foo if you're going to dump it all into foo anyway?) but not all uses at namespace scope are like that.

@Aster89
Copy link
Author

Aster89 commented Sep 28, 2024

So maybe what you're asking for is for SF.7 to talk about namespace scope

Yeah, sorry, I thought I had written it somewhere! Yes, I'm suggesting a change along the lines of

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

As regards the snippet

namespace mine {
  using namespace yours;
}

what would a justifiable usecase be?

Let me clarify what I mean by "justifiable". If the motivation for the snippet above in a header file was "I'm writing stuff in namespace mine {} and I don't want to type yours::, because I call a lot of these functions/declare a lot of variabels of those types, ...", I wouldn't consider that justifiable. The simple "a lot"-s would make me thing we're talking of a huge header in the first place, so why not splitting it?, how many things is it doing that could belong to different headers?, ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants