-
Notifications
You must be signed in to change notification settings - Fork 56
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
Ifdef t8 enable script #1375
Ifdef t8 enable script #1375
Conversation
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.
Great and awesome that you did it so quickly.
Some minor changes suggested.
@Davknapp Can we also throw this in our spell checker CI? |
I propose to add this to our typos script so that it is executed by the CI. |
That is not easily possible, because it it not really a typo and this is not what the intended use of the script is. Maybe we could hack it somehow, but I prefer @sandro-elsweijer suggestion. |
sure |
before it was only updated in a subshell. This way the variable is updated correctly
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.
Very nice commenting :)
Only minor change left: the file name of the artifact.
"If this PR introduces a new feature, it must be covered in an example/tutorial and a Wiki article." We should adress this by documenting the usage of ENABLE_IF in our coding guidelines and linking to the new script. "The author added the patch/minor/major label in accordance to semantic versioning." |
Closes #1374
Checks in each file if a
#ifdef T8_ENABLE_
is used instead of a#if T8_ENABLE_
All these boxes must be checked by the reviewers before merging the pull request:
As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.
General
The reviewer executed the new code features at least once and checked the results manually
The code follows the t8code coding guidelines
New source/header files are properly added to the Makefiles
The code is well documented
All function declarations, structs/classes and their members have a proper doxygen documentation
All new algorithms and data structures are sufficiently optimal in terms of memory and runtime (If this should be merged, but there is still potential for optimization, create a new issue)
Tests
Github action
The code compiles without warning in debugging and release mode, with and without MPI (this should be executed automatically in a github action)
All tests pass (in various configurations, this should be executed automatically in a github action)
If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):
Scripts and Wiki
script/find_all_source_files.scp
to check the indentation of these files.License
doc/
(or already has one)Tag Label