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

Cleanup: remove using namespace std; #3016

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

arrufat
Copy link
Contributor

@arrufat arrufat commented Sep 19, 2024

Fixes #3015

@ChristopheDumeunier
Copy link

Thanks for the quick reaction. This fixes my problem.
Note that you may encounter the same problem with some 'using namespace std::chrono' in some headers too. ;-)

@arrufat
Copy link
Contributor Author

arrufat commented Sep 19, 2024

Yes, I saw a couple of those, too. Might fix it later.

@arrufat
Copy link
Contributor Author

arrufat commented Sep 19, 2024

Not too sure about this one ab82c4f

I am ready to revert it.

@davisking
Copy link
Owner

Eh, what is the specific error we are talking about @ChristopheDumeunier ? Because doing using namespace inside a function scope is totally fine. It doesn't matter if that function is in a header or not. What's bad is doing it at a global scope in a header, as that brings symbols into client code.

Was there any of that in dlib? There shouldn't be.

I don't really want to go mass changing code that isn't breaking. That's invariably how we find out later on some platform something is now broken.

@ChristopheDumeunier
Copy link

ChristopheDumeunier commented Sep 20, 2024

@davisking Doing using namespace inside a function is not fine if the inline/template function is defined in a header because you don't know the compilation context of your code.

Here is a code that reproduce the problem: https://godbolt.org/z/r7T5nqMr5
error: reference to 'numeric_limits' is ambiguous

@arrufat
Copy link
Contributor Author

arrufat commented Sep 20, 2024

So, in that example, you declare your namespace in a CPP file and include dlib after that?

If you just move the code that simulates including dlib to the top of the C++ (where the #include directives usually are), the error is gone.

I am just curious, why would you do that?

@ChristopheDumeunier
Copy link

It's a reduced example. In our application, some parts of the unit tests of the code are generated by scripts and macro, the the order of includes can be strange and vary depending on (de)activated dependencies. We use dlib from a long time, without this error. The error appeared when we added a struct named "numeric_limits" somewhere else in our namespace.

@arrufat
Copy link
Contributor Author

arrufat commented Sep 20, 2024

The using namespace thing in dlib isn't the issue, if you name anything like something that's already inside dlib and then use using namespace <your_namespace> before including dlib, you'll have the same problem.

That's why they say you shouldn't use them globally in header files: so that you don't clash with other people's code.

But what you're doing amounts to that: using namespace before importing a header is akin to using it globally in a header file.

I am closing this PR.

@arrufat arrufat closed this Sep 20, 2024
@davisking
Copy link
Owner

davisking commented Sep 20, 2024

Well, it's not exactly the same @arrufat

Since name lookup is preferred for names in your own namespace. Like if you did class matrix{}; before including any dlib headers it's still fine and nothing bad would happen, since dlib is all in the one monolithic dlib namespace any code in it will still prefer the dlib::matrix. But for symbols outside the dlib namespace, if someone goes and defines them at global scope like in the example yeah it's going to become ambiguous.

Which is generally a thing you shouldn't be doing. But that's fine, dlib is all about being convenient for users, so making these changes is fine with me. I get that people get pushed into doing stuff that is sub-optimal in software and we should make those cases not a pain when we can :)

That is to say, I just wanted to know if this was a real problem from real code and work or a "if I break it on purpose it's broken" example. It's with real code, so let's make that real code work since it's probably not going to mess up anything in dlib anyway :)

@davisking davisking reopened this Sep 20, 2024
@arrufat
Copy link
Contributor Author

arrufat commented Sep 20, 2024

I see. Actually, that's why I hurried to make this PR so that I could solve @ChristopheDumeunier's issue.

However, you scared me that I might have just broken some quirky compilers/setups by doing that stuff (I saw workarounds for GCC 4.9 while doing this PR).

And I wouldn't want to take more of your time with more people opening issues because of me...

@davisking
Copy link
Owner

I see. Actually, that's why I hurried to make this PR so that I could solve @ChristopheDumeunier's issue.

However, you scared me that I might have just broken some quirky compilers/setups by doing that stuff (I saw workarounds for GCC 4.9 while doing this PR).

Yeah, maybe just leave that one. IDK. It's an old compiler so I doubt anyone cares.

And in general, the easy thing to do is often to replace any using namespace std; with a using std::whatever_the_thing_is_its_using; and then leave the rest of the code alone. Kinda depends on how many lines need to change and how readable/unreadable it makes them. Like the chrono stuff gets kinda unreadable if it's all fully scoped on each use.

@@ -18,7 +18,6 @@ namespace
{

using namespace test;
using namespace std;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to edit any of the stuff in dlib/test. That is all fine since nothing should be including it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I guess it doesn't hurt anything to do it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought, since, I was at it, we might as well do it in the same PR... I can revert those if you want.

@arrufat
Copy link
Contributor Author

arrufat commented Sep 20, 2024

I see. Actually, that's why I hurried to make this PR so that I could solve @ChristopheDumeunier's issue.
However, you scared me that I might have just broken some quirky compilers/setups by doing that stuff (I saw workarounds for GCC 4.9 while doing this PR).

Yeah, maybe just leave that one. IDK. It's an old compiler so I doubt anyone cares.

I think it's safe to remove, as dlib now uses C++14, it doesn't look like GCC 4.9 fully supports C++14: https://en.cppreference.com/w/cpp/compiler_support/14
GCC 5 was the first version to fully support C++14.

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

Successfully merging this pull request may close these issues.

[Bug]: Conflicts with other codes due to "using namespace std;" in header files
3 participants