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

Updates to type alias for RayTracer and double-down namespace. #786

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

pshriwise
Copy link
Member

I added a namespace to double-down in pshriwise/double-down#29 to protect from clashes of commonly-used class names (Vec3f, etc.) and constants with other packages. These are breaking changes with DAGMC that are resolved in this PR. I figured it would be good to get this relatively small issue taken care of before our next DAGMC release.

To ease potential concerns about any continued patterns along these lines, I'm about to do an initial release of double-down. No further breaking changes should happen before that, meaning that DAGMC can rely on specific versions of dd for testing going forward.

Description

Movement of double-down includes from DagMC.cpp to DagMC.hpp.

Motivation and Context

To maintain compatibility with the latest version of double-down.

@shimwell
Copy link
Member

shimwell commented Nov 9, 2021

+1 for merging this in before the new DAGMC release

@shimwell
Copy link
Member

shimwell commented Nov 9, 2021

Changes are showing up on CI when building DAGMC with Double Down Screenshot from 2021-11-09 21-10-14

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Just one little design query..

@@ -29,8 +33,6 @@ struct DagmcVolData {
std::string comp_name;
};

class RayTracingInterface;
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to forward declare this in a namespace to avoid moving the include here? Not sure if that works, but it would be good to avoid includes in headers if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure can! Good call.

@pshriwise
Copy link
Member Author

pshriwise commented Nov 10, 2021

Changes are showing up on CI when building DAGMC with Double Down Screenshot from 2021-11-09 21-10-14

Is this in a DAGMC action or an external project?

@shimwell
Copy link
Member

Changes are showing up on CI when building DAGMC with Double Down Screenshot from 2021-11-09 21-10-14

Is this in a DAGMC action or an external project?

Just a little external project. (Neutronics-workshop)

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

This looks good to me...

@gonuke
Copy link
Member

gonuke commented Nov 10, 2021

I can't tell if @shimwell's concerns should block merging or not?

@shimwell
Copy link
Member

I can't tell if @shimwell's concerns should block merging or not?

No concerns about the PR over here. I'm keen to merge ASAP so I can get a few workflows running again

@gonuke gonuke merged commit fbd0cdb into svalinn:develop Nov 11, 2021
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