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 superfluous headers from DAGMC headers #800

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

gonuke
Copy link
Member

@gonuke gonuke commented Feb 20, 2022

Description

Move headers from DAGMC's headers to the implementation files. Upstream headers only need to be included in header files if details of the classes that those upstream headers define are accessed within the header. If only the existence of those classes is necessary, then forward declaration of the classes is sufficient.

Motivation and Context

Since DAGMC's headers may be included by downstream codes, it is good practice to avoid "polluting" our headers with other headers that may, unkown to us, further include other headers and so on. This

Fixes #799

Changes

Fix of an annoying but non-pathologial bug

@gonuke gonuke marked this pull request as draft February 20, 2022 22:00
@gonuke
Copy link
Member Author

gonuke commented Feb 20, 2022

Converted this to draft because it will be more challenging than originally expected.

@gonuke gonuke force-pushed the header_cleanup branch 2 times, most recently from 0534936 to ddf711a Compare August 15, 2023 21:07
@ahnaf-tahmid-chowdhury
Copy link
Member

I think we can move this branch to upstream and give this a update?

@gonuke
Copy link
Member Author

gonuke commented Feb 15, 2024

Why move to upstream?

@ahnaf-tahmid-chowdhury
Copy link
Member

Not mandatory, as we don't need to work on Dockerfile. But, couldn't it be easy for me to fork? Or I may add your remote branch to my repo.

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.

Remove need for downstream codes to have Eigen3 installed
2 participants