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

Reorganising the MPI code and adding a default main() entry to the MPI #677

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

jokervTv
Copy link
Contributor

Dear, sir

  • Fixed linker error by restructuring the code when tried to use cmake to integrate doctest with mpi.
  • Standardising the style of the code using project .clang-format.
  • Used 'Include Guards' instead of #program once to improve compatibility.
  • Added macro DOCTEST_CONFIG_IMPLEMENT_WITH_MPI_MAIN to provide a default main entry for mpi tests.
  • Markdown documentation has also been updated.

Thanks for open sourcing this useful library.

@codecov
Copy link

codecov bot commented Jul 31, 2022

Codecov Report

Merging #677 (b70116e) into dev (b7c21ec) will not change coverage.
The diff coverage is n/a.

@@           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.

@Saalvage
Copy link
Member

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?

Copy link
Member

@philbucher philbucher left a 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"
Copy link
Member

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?

Copy link
Contributor Author

@jokervTv jokervTv Aug 5, 2022

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?

doctest/extensions/mpi_sub_comm.h Outdated Show resolved Hide resolved
doctest/extensions/doctest_mpi.h Outdated Show resolved Hide resolved
doctest/extensions/mpi_reporter.h Outdated Show resolved Hide resolved
jokervTv and others added 3 commits August 5, 2022 12:49
Optimising macro names

Co-authored-by: Philipp Bucher <philipp.bucher@tum.de>
Optimising macro names

Co-authored-by: Philipp Bucher <philipp.bucher@tum.de>
@jokervTv
Copy link
Contributor Author

jokervTv commented Aug 5, 2022

Thank you for your comments.
I need some time to think about how to separate PR.

@starintheuniverse
Copy link
Contributor

Agreed, it's great to have more eyes and hands on the MPI extension!
I will also try to take a closer look at this over the weekend.

@jokervTv

  • Fixed linker error by restructuring the code when tried to use cmake to integrate doctest with mpi.

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.

@jokervTv
Copy link
Contributor Author

jokervTv commented Aug 6, 2022

Dear Sir,

I have separated(in order) the PR to

I had delete Standardising the style of the code using project .clang-format, this may be better to review.
And how to handle this PR, Just close?

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.

None yet

4 participants