-
Notifications
You must be signed in to change notification settings - Fork 62
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
Breaking changes with PCU #433
base: develop
Are you sure you want to change the base?
Conversation
This commit adds a MPI state that's separate from the global variables to the internal pcu_mpi interfaces. The user level PCU interface remains unchanged. However, this change lays the groundwork for PCU2 that will not make use of global state. This is important for some use cases like working around the spectrum-mpi comm_dup bug. The removal of global state also improves testability and modularity of this code.
Throughout the code base it was assumed that the mpi functions were implemented through pmpi and so no other implementation really could have been implemented. This change just removes the complexity from having the vtable.
This commit allows the pcu_mpi to be initialized into a stack variable rather than allways allocating on the heap.
This commit adds a MPI state that's separate from the global variables to the internal pcu_mpi interfaces. The user level PCU interface remains unchanged. However, this change lays the groundwork for PCU2 that will not make use of global state. This is important for some use cases like working around the spectrum-mpi comm_dup bug. The removal of global state also improves testability and modularity of this code.
Throughout the code base it was assumed that the mpi functions were implemented through pmpi and so no other implementation really could have been implemented. This change just removes the complexity from having the vtable.
This commit allows the pcu_mpi to be initialized into a stack variable rather than allways allocating on the heap.
@cwsmith, I also changed the file naming of PCUObj.h and PCU.h, and added explicitly declared data types to the templated add, min, max functions. I'm not seeing in the reviews any other changes we'd talked about making, but let me know if you'd like anything done differently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found at least some instances of mismatched types between the templated functions and the original ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thank you.
@flagdanger Do you have the output from the delta wing case?
Besides the comments/edits below, we need to capture a summary of the changes in the opening comment so it can be added to the merge commit.
pcu/pcu_c.cc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was git mv
used to create this file from pcu/pcu.c
? The github PR interface it shows pcu/pcu.c
as being deleted and pcu/pcu_c.cc
as a new file. It would be nice to maintain the history of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flagdanger I'd like to maintain the linkage between these files so history is preserved. Please let me know if you need help with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I forgot. I might need help doing this, I was having trouble before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. No problem. I'll message you offline to setup a time.
/runtests |
|
Update:
/runtests |
Simmetrix + CGNS Test Result: failure |
It looks like adding the C++14 standard into the mix for the cgns build causes some more errors to be emitted: |
@flagdanger looks like that error is just an extra semicolon |
@jacobmerson Could you try running tests again? |
/runtests |
Simmetrix + CGNS Test Result: failure |
/runtests |
Build Log |
@cwsmith Could you run tests again? |
/runtests |
Build Log |
@flagdanger @cwsmith Is all that's left fixing merge conflicts and rerunning the tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I saw only one left over unique_ptr
use.
@@ -98,7 +97,7 @@ int main(int argc, char** argv) | |||
auto PCUObj = std::unique_ptr<pcu::PCU>(new pcu::PCU(MPI_COMM_WORLD)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the other examples the unique_ptr was removed, I assume that can be done here too?
Yeah, and we need to have a summary of the changes in the opening comment so it can be recorded in the merge commit. |
Breaking changes with PCU