diff options
Diffstat (limited to 'contributing.md')
| -rw-r--r-- | contributing.md | 155 |
1 files changed, 136 insertions, 19 deletions
diff --git a/contributing.md b/contributing.md index 77a2908..d217410 100644 --- a/contributing.md +++ b/contributing.md @@ -17,6 +17,23 @@ that you can click on to open them. working/compiling version of the project - Pull requests for new code include either automated tests for the new code or an explanation as to why the code can not (reliably) be tested +- Non-bugfix pull requests must be approved by at least 2 reviewers before being + merged +- Pull requests should have the following labels (where appropriate) + |label|meaning| + |:-:|-| + |`fix me`|has feedback that should be resolved/discussed by its author| + |`review me`|needs additional reviewers (minimum of 2 per PR)| + |`do not review`|is actively being worked on or not ready for feedback| + |`high priority`|should be worked on before all the others| + - PRs start with the `review me` label + - Reviewers— + - Add the `fix me` label after adding comments + - Authors— + - Remove the `review me` label if the pull request has enough reviewers + - Add the `do not review` label while processing feedback / pushing + additional commits + <!-- - TODO: tagging / versions --> @@ -160,40 +177,28 @@ that you can click on to open them. ``` </td></tr></table></details> - <details><summary> - <code>using namespace</code> may not be used in header files (.h, .hpp), only - in source files (.cpp). + <a href="https://en.cppreference.com/w/cpp/language/using_declaration">Using-declarations</a> + may not be used in header files (<code>.h</code>, <code>.hpp</code>), only in + source files (<code>.cpp</code>). </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> example.h: ```cpp namespace crepe { - void foo(); + std::string foo(); } ``` - example.cpp: - ```cpp - #include "example.h" - using namespace crepe; - void foo() {} - ``` </td><td> example.h: ```cpp + using namespace std; + namespace crepe { - template <typename T> - T foo(); + string foo(); } ``` - - example.hpp: - ```cpp - #include "example.h" - using namespace crepe; - template <typename T> - T foo(); - ``` </td></tr></table></details> - <details><summary> @@ -495,6 +500,12 @@ that you can click on to open them. </td></tr></table></details> - <details><summary> Ensure const-correctness + + > [!IMPORTANT] + > C-style APIs that work on (possibly internal) references to structs can be + > called from const member functions in C++. If the compiler allows you to + > mark a function as `const` even though it has side effects, it should + > **not** be marked as `const`. </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> ```cpp @@ -626,15 +637,21 @@ that you can click on to open them. </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> ```cpp + void Foo::bar() { } + void Foo::set_value(int value) { this->value = value; + this->bar(); } ``` </td><td> ```cpp + void Foo::bar() { } + void Foo::set_value(int new_value) { value = new_value; + bar(); } ``` </td></tr></table></details> @@ -795,6 +812,49 @@ that you can click on to open them. ``` </td></tr></table></details> - Do not implement new classes as singletons +- <details><summary> + Retrieving the first or last indices for iterators with a known or expected + size should be done using <code>.front()</code> or <code>.back()</code> + instead of by index + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + vector<int> foo = { 1, 2, 3 }; + int bar = foo.first(); + ``` + </td><td> + + ```cpp + vector<int> foo = { 1, 2, 3 }; + int bar = foo[0]; + ``` + </td></tr></table></details> +- <details><summary> + Always explicitly check against <code>NULL</code> (for C APIs) or + <code>nullptr</code> (for C++ APIs) when checking if a pointer is valid + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + string foo = "Hello world"; + if (foo.c_str() == nullptr) + // ... + + void * bar = malloc(); + if (bar == NULL) + // ... + ``` + </td><td> + + ```cpp + string foo = "Hello world"; + if (!foo.c_str()) + // ... + + void * bar = malloc(); + if (!bar) + // ... + ``` + </td></tr></table></details> ## CMakeLists-specific @@ -823,6 +883,11 @@ that you can click on to open them. parameter of `TEST()` / `TEST_F()` macro) - Test source files match their suite name (or test fixture name in the case of tests that use a fixture) +- Tests that measure time or use delays must be [disabled][gtest-disable] (by + prepending `DISABLED_` to the suite or case name). + + These tests will still be compiled, but will only run when the `test_main` + binary is run with the `--gtest_also_run_disabled_tests` flag. # Structure @@ -842,6 +907,8 @@ that you can click on to open them. # Documentation +[Doxygen commands](https://www.doxygen.nl/manual/commands.html) + - All documentation is written in U.S. English - <details><summary> Doxygen commands are used with a backslash instead of an at-sign. @@ -927,6 +994,52 @@ that you can click on to open them. Foo & operator=(Foo &&) = delete; ``` </td></tr></table></details> +- Do not use markdown headings in Doxygen + +## Documenting features + +Engine features are small 'building blocks' that the user (game developer) may +reference when building a game with the engine. Features do not necessarily map +1-1 to engine components or systems. If a component or system has a single, +distinct feature it should be named after that feature, not the component or +system itself. + +The sources for these pages are located under `src/doc/feature/`, and have the +following format: + +- A feature description which explains— + - the purpose and function of the feature (focus on what it enables or + achieves for the user) + - additional information about when to implement the feature, such as specific + use cases or scenarios +- A list of 'see also' references to relevant classes and/or types +- A **minimal** example to demonstrate how the feature is used. The example + should be written such that the following is clear to the reader: + - Which headers need to be included to utilize the feature + - *Why* the example works, not what is happening in the example + - Where is this code supposed to be called (e.g. inside scene/script + functions) + - Which restrictions should be kept in mind (e.g. copy/move semantics, max + component instances, speed considerations) + +Features should be documented as clear and concise as possible, so the following +points should be kept in mind: + +- <details><summary> + If a page expands on an example from another page, directly reference the + other page using a cross-reference (`\ref`) in a `\note` block at the top of + the page. + </summary> + + ``` + \note This page builds on top of the example shown in \ref feature_script + ``` + </details> +- When explaining the usage of specific functions, qualify them such that + Doxygen is able to add a cross-reference or manually add a reference using the + `\ref` command. +- Users will likely copy-paste examples as-is, so do this yourself to check if + the example code actually works! # Libraries @@ -934,4 +1047,8 @@ that you can click on to open them. subdirectory - When adding new submodules, please set the `shallow` option to `true` in the [.gitmodules](./.gitmodules) file +- When adding new libraries, please update the library version table in + [readme\.md](./readme.md) + +[gtest-disable]: https://google.github.io/googletest/advanced.html#temporarily-disabling-tests |