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

Remove using namespace Lib from hpp files. #588

Closed
wants to merge 1 commit into from

Conversation

kharus
Copy link
Contributor

@kharus kharus commented Aug 13, 2024

This PR removes using namespace Lib; from hpp files. And either prepends Lib:: or adds using namespace Lib; in cpp files.
Similar to #472 from last year.
@MichaelRawson

@MichaelRawson
Copy link
Contributor

Thanks for doing this @kharus! using namespace in header files is a bit violent and something we really ought to avoid, so I am keen to merge this.

The only real downside is that sometimes the headers get verbose, especially if they have inline routines in them. If @quickbeam123 has some objection to specific code, we could use the following pattern:

namespace Bar {
  namespace USING_DECLS {
    using namespace Lib;
    class Foo { Stack<int> whatever; };  
  }
  use USING_DECLS::Foo;
}

// now have Bar::Foo available

@kharus
Copy link
Contributor Author

kharus commented Aug 14, 2024

I agree, maybe worth taking a pause here and consider wider implications.
As there are more namespaces imported in headers and declarations are getting longer and longer with more visual noise from namespaces::.

@quickbeam123
Copy link
Collaborator

I understand and appreciate the effort. I was also tempted several times to "do things the right way".

But I don't think such efforts are worth it. Touching 236 files without any performance improvement and just to "do things the right way", with the downsides of less effective git blame, tiny chance of introducing a bug, every live feature branch having to eventually adopt this and me having to look at all these 236 (yes, I do that), does not feel to me like a net gain.

After all, with namespaces and using, since we are in full control of the whole vampire codebase, imagine there was no namespace Lib, we could write short stuff everywhere (just like with using in the headers), and if there was a clash with a library, we would just rename our class. How about we think of our namespaces in this weaker sense, as some convenience compartments corresponding to the folder structure, but not as a foolproof device against name clashes?

Idea for a general policy: Do things the right way in modules you touch for some more important reasons. Even change that module fully, such that you really like it. But try to avoid large refactors only for the pleasure of "finally being in peace with it". (That's what I try to do with the trailing whitespaces ever since I installed the vscode extension ;))

@kharus
Copy link
Contributor Author

kharus commented Aug 14, 2024

@quickbeam123 I agree with your points. Closing.

@kharus kharus closed this Aug 14, 2024
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.

3 participants