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

Include utilities namespace wherever necessary #6217

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

gassmoeller
Copy link
Member

This PR fixes an issue that appeared in #6215: Not all of ASPECT's namespaces named Utilities contain a directive using namespace dealii::Utilities. This can lead to hard to understand error messages like that Utilities::MPI:... cannot be found, even if the ASPECT file in question was not changed at all (e.g. because it was grouped into a different unity build file that contained a new Utilities namespace). This PR introduces using namespace dealii::Utilities into every file that introduces an Utility namespace.

@gassmoeller gassmoeller force-pushed the fix_utility_namespaces branch from 41bdc86 to c75dfb8 Compare February 2, 2025 15:12
@gassmoeller gassmoeller force-pushed the fix_utility_namespaces branch from c75dfb8 to 0d886ce Compare February 2, 2025 15:13
@gassmoeller
Copy link
Member Author

I introduced a change after noticing a problem in #6215: There are two separate cases:

  1. Every header that adds something to aspect::Utilities needs to make sure dealii::Utilities is included-
  2. Every header that introduces a new namespace named Utilities needs to make sure the namespace aspect::Utilities is included.

My previous solution of simply including dealii::Utilities in both cases above would not make the aspect specific functions in aspect::Utilities available.

I updated the code and added a comment to make sure this logic is documented.

The logic for all symbols in the Utilities namespaces now look approximately like this:

dealii::Utilities -> aspect::Utilitiies -> aspect::specific_namespace::Utilities 

Not the prettiest solution, but at least this way we should avoid all future problems with these namespaces. The alternative was to specify for each use of Utilities::... if it was part of aspect or deal.II (I checked there are ~1300 cases XD).

@tjhei: Can you take another look? Sorry we overlapped with your review and my modification.

@tjhei
Copy link
Member

tjhei commented Feb 3, 2025

Maybe we should get rid of the aspect::bla::Utilities namespaces. I find that quite confusing.

@tjhei tjhei merged commit 752d6ad into geodynamics:main Feb 3, 2025
8 checks passed
@gassmoeller
Copy link
Member Author

Maybe we should get rid of the aspect::bla::Utilities namespaces. I find that quite confusing.

We could, I wouldnt be opposed to it. The material model already uses aspect::MaterialModel::MaterialUtilities, which prevents confusion.

@gassmoeller gassmoeller deleted the fix_utility_namespaces branch February 3, 2025 09:56
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