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

Improve dealii namespace inclusion #6219

Merged

Conversation

gassmoeller
Copy link
Member

This is something I noticed while working on #6217: About half of our header files (and some source files) have the directive using namespace dealii; in their respective namespaces (or sometimes in the main aspect namespace). The other half doesnt, and relies on some header file including the namespace. I think it would make sense to unify this. In particular, since all of our files are expected to include global.h either directly or through their headers, I think it would make sense to only include the deal.II namespace in aspect/global.h, and make sure the dealii namespace is always included in the aspect namespace. This lowers the likelihood of name collision in all namespaces except for the global aspect namespace itself and hides a directive that many new users (familiar with C or python) will not understand. It also reduces the number of lines of code.

An alternative to this PR would be to make sure every file includes the dealii namespace in an appropriate place.

All of the code changes are very straightforward, except:

  • I had to include using namespace dealii also in compat.h (likely because compat.h and global.h have a circular include
  • The change revealed a bug/unintended use of namespaces in source/postprocess/particle_count_statistics.cc that I fixed
  • I had to add a namespace aspect in a number of source files of tests that previously did everything in the global namespace. This adds a lot of whitespace change in those files. Not sure this is the cleanest solution, but I think it is reasonable to except all ASPECT code to live in the aspect namespace.
  • I left out unity tests. They all do stuff in the global namespace and use their own directives around namespaces

Copy link
Member

@tjhei tjhei left a comment

Choose a reason for hiding this comment

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

I think being consistent is much better than the current state. Thank you for going through every file.

I still think it was a big mistake including dealii, but that ship has sailed a long time ago.

@tjhei tjhei merged commit e148692 into geodynamics:main Jan 31, 2025
8 checks passed
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.

2 participants