-
-
Notifications
You must be signed in to change notification settings - Fork 622
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
Reorganising the MPI code and adding a default main() entry to the MPI #677
base: dev
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #677 +/- ##
=======================================
Coverage 92.19% 92.19%
=======================================
Files 2 2
Lines 2127 2127
=======================================
Hits 1961 1961
Misses 166 166 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This is a huge PR, I hope you understand that it will likely take some time to be merged. @philbucher Do you think you will have the time to take a look at this anytime soon? |
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.
First of all thanks for your contributions, I like to see that the MPI-version of doctest gains traction!
A few general comments. It seems like this PR does multiple things. Especially the reformatting makes it very hard to review. If possible, can you please make a separate PR for them to simplify the review of this one? You are making sensible changes to the code which need to be well understood, since we had problems not long ago. (@Saalvage what is the current way of doing code-formatting? Is it part of the CI?)
@BerengerBerthoul @starintheuniverse you also use this, can you please also have a look?
@@ -1,6 +1,7 @@ | |||
#define DOCTEST_CONFIG_IMPLEMENT | |||
|
|||
#include <doctest/extensions/doctest_mpi.h> | |||
#include "mpi.cpp" |
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.
Including source files is usually not recommend
Can you please explain your reasoning?
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.
the mpi.cpp
is a test file with DOCTEST_CONFIG_IMPLEMENT_WITH_MPI_MAIN
in same folder, using it to test the mpi function with default main()
. I think this approach allows for new tests to be simply added later, just need once in mpi.cpp
.
is it a good way to change the file name or add new header file here?
Optimising macro names Co-authored-by: Philipp Bucher <philipp.bucher@tum.de>
Optimising macro names Co-authored-by: Philipp Bucher <philipp.bucher@tum.de>
Thank you for your comments. |
Agreed, it's great to have more eyes and hands on the MPI extension!
as you are organizing/annotating the PR, I think it would be helpful to describe your build system and the linker error in a bit more detail to make it reproducible. I didn't run into issues with CMake before so I am curious. |
Dear Sir, I have separated(in order) the PR to
I had delete |
Dear, sir
.clang-format
.#program once
to improve compatibility.DOCTEST_CONFIG_IMPLEMENT_WITH_MPI_MAIN
to provide a default main entry for mpi tests.Thanks for open sourcing this useful library.