diff options
Diffstat (limited to 'contributing.md')
| -rw-r--r-- | contributing.md | 624 |
1 files changed, 615 insertions, 9 deletions
diff --git a/contributing.md b/contributing.md index 2fe46f7..5555892 100644 --- a/contributing.md +++ b/contributing.md @@ -15,12 +15,19 @@ that you can click on to open them. `name/feature` (i.e. `loek/dll-so-poc` or `jaro/class2`) - The master branch is considered stable, and should always contain a 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 + +<!-- - TODO: tagging / versions +--> # Code style - Formatting nitty-gritty is handled by clang-format/clang-tidy (run `make - format` in the root folder of this repository to format all sources files) + format` or `make lint`) - <details><summary> ASCII only </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> @@ -49,11 +56,11 @@ that you can click on to open them. class Cars {}; ``` </td></tr></table></details> -- Source files contain the following types of comments: +- Source files (`.cpp`, `.hpp`) contain the following types of comments: - What is the code supposed to do (optional) - Implementation details (if applicable) -- Header files contain the following types of comments: - - Usage documentation (required) +- Header files (`.h`) contain the following types of comments: + - [Usage documentation](#documentation) (required) - Implementation details (if they affect the header) - Design/data structure decisions (if applicable) - <details><summary> @@ -76,8 +83,8 @@ that you can click on to open them. } ``` </td></tr></table></details> -- Header includes are split into paragraphs separated by a blank line. The - order is: +- Header includes (at the top of files) are split into paragraphs separated by + a blank line. The order is: 1. system headers (using `<`brackets`>`) 2. relative headers NOT in the same folder as the current file 3. relative headers in the same folder as the current file @@ -110,7 +117,54 @@ 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, only in source files. + If there is one, the matching template header (<code>.hpp</code>) is included + at the bottom of the regular header (<code>.h</code>) + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + Foo.h: + ```cpp + #pragma once + + template <typename T> + void foo(); + + #include "Foo.hpp" + ``` + + Foo.hpp: + ```cpp + #pragma once + #include "Foo.h" + + template <typename T> + void foo() { + // ... + } + ``` + </td><td> + + Foo.h: + ```cpp + #pragma once + + template <typename T> + void foo(); + ``` + + Foo.hpp: + ```cpp + #pragma once + #include "Foo.h" + + template <typename T> + void foo() { + // ... + } + ``` + </td></tr></table></details> +- <details><summary> + <code>using namespace</code> may not be used in header files (.h, .hpp), only + in source files (.cpp). </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> example.h: @@ -277,7 +331,7 @@ that you can click on to open them. ```cpp struct Foo { - int bar; + int bar = 0; std::string baz; }; ``` @@ -285,7 +339,7 @@ that you can click on to open them. ```cpp struct Foo { - int bar = 0; + int bar; std::string baz; }; ``` @@ -386,6 +440,419 @@ that you can click on to open them. #endif ``` </td></tr></table></details> +- <details><summary> + Variables that are being moved always use the fully qualified + <code>std::move</code> + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + using namespace std; + string foo = "bar"; + ref_fn(std::move(foo)); + ``` + </td><td> + + ```cpp + using namespace std; + string foo = "bar"; + ref_fn(move(foo)); + ``` + </td></tr></table></details> +- <details><summary> + If possible, classes and structs are passed to functions by (const) reference + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + void foo(const Point & p); + ``` + </td><td> + + ```cpp + void foo(Point & p); + void bar(Point p); + ``` + </td></tr></table></details> +- <details><summary> + Follow the rule of five + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + class Foo { + public: + Foo(); + ~Foo(); + Foo(Foo &&) noexcept; + Foo & operator = (const Foo &); + Foo & operator = (Foo &&) noexcept; + }; + ``` + </td><td> + + ```cpp + class Foo { + public: + Foo(); + ~Foo(); + }; + ``` + </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 + class Foo { + public: + int get_value() const; + void set_value(int new_value); + const std::string & get_name() const; + void set_name(const std::string & new_name); + private: + int value; + std::string name; + }; + ``` + </td><td> + + ```cpp + class Foo { + public: + int get_value(); + void set_value(int new_value); + std::string get_name(); + void set_name(std::string new_name); + private: + int value; + std::string name; + }; + ``` + </td></tr></table></details> +- <details><summary> + Files should be named after the class/struct/interface they implement + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + MyClass.h + MyClass.cpp + MyClass.hpp + ``` + </td><td> + + ```cpp + my_class.h + myClass.cpp + my-class.hpp + ``` + </td></tr></table></details> +- <details><summary> + Implementations are not allowed in header files, except if the implementation + + - is `= default` + - is `= delete` + - is `{}` (empty) + - only returns a constant literal + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + class Foo { + public: + int get_value() const { return 42; } + }; + ``` + </td><td> + + ```cpp + class Foo { + public: + int calculate_value() const { + int result = 0; + // complex calculation + return result; + } + }; + ``` + </td></tr></table></details> +- <details><summary> + Use angle brackets (<code><></code>) only for including system headers and + double quotes (<code>""</code>) for including other engine files. + + > [!NOTE] + > Only files in the examples folder should include engine headers with angle + > brackets + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + #include <iostream> + + #include "facade/Sound.h" + ``` + </td><td> + + ```cpp + #include <iostream> + #include <crepe/facade/Sound.h> + ``` + </td></tr></table></details> +- <details><summary> + Ensure exception safety by using RAII classes + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + auto foo = std::make_unique<Foo>(); + ``` + </td><td> + + ```cpp + Foo* foo = new Foo(); + // ... + delete foo; + ``` + </td></tr></table></details> +- <details><summary> + Do not use C-style memory management APIs (<code>malloc</code>, + <code>calloc</code>, <code>free</code>) + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + Foo * foo = new Foo(); + delete foo; + ``` + </td><td> + + ```cpp + Foo * foo = (Foo *) malloc(sizeof(Foo)); + free(foo); + ``` + </td></tr></table></details> +- <details><summary> + Prefix all class members with <code>this-></code> + </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> +- <details><summary> + Assigning booleans should be done with the + <code>true</code>/<code>false</code> literals instead of + <code>0</code>/<code>1</code> + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + bool foo = true; + bool bar = false; + ``` + </td><td> + + ```cpp + bool foo = 1; + bool bar = 0; + ``` + </td></tr></table></details> +- <details><summary> + The reason for <code>friend</code> relations are documented + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + //! ComponentManager calls the private constructor of this class + friend class ComponentManager; + ``` + </td><td> + + ```cpp + friend class ComponentManager; + ``` + </td></tr></table></details> +- <details><summary> + Do not <i>pick</i> fixed-width integer types (unless required) + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + unsigned long long foo(); + ``` + </td><td> + + ```cpp + uint64_t foo(); + ``` + </td></tr></table></details> +- <details><summary> + Utilize standard exceptions where appropriate (those found in <code><stdexcept></code>) + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + #include <stdexcept> + + // ... + + if (foo == nullptr) { + throw std::runtime_error("What is wrong"); + } + ``` + </td><td> + + ```cpp + if (foo == nullptr) { + std::cout << "What is wrong" << std::endl; + exit(1); + } + ``` + </td></tr></table></details> +- <details><summary> + Mention the name of the class when throwing an exception + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + Foo::bar() { + if (...) + throw std::runtime_error("Foo: big error!"); + } + ``` + </td><td> + + ```cpp + Foo::bar() { + if (...) + throw std::runtime_error("big error!"); + } + ``` + </td></tr></table></details> +- <details><summary> + Constructors of classes derived from <code>Component</code> should be + protected and <code>ComponentManager</code> should be declared as a friend + class. + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + class MyComponent : public Component { + protected: + MyComponent(...); + //! Only ComponentManager is allowed to create components + friend class ComponentManager; + }; + ``` + </td><td> + + ```cpp + class MyComponent : public Component { + public: + MyComponent(...); + }; + ``` + </td></tr></table></details> +- <details><summary> + C++ <code>std::format</code> should be used instead of C-style format specifiers + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + std::string message = std::format("Hello, {}", name); + + dbg_logf("Here too: {}", 3); + + throw std::runtime_error(std::format("Or here: {}", 5)); + ``` + </td><td> + + ```cpp + char message[50]; + sprintf(message, "Hello, %s", name); + ``` + </td></tr></table></details> +- <details><summary> + Argument names should be added in <code>.h</code> files (not only in + <code>.cpp</code> and <code>.hpp</code> files) + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + Foo.h: + ```cpp + void foo(int bar); + ``` + + Foo.cpp: + ```cpp + void foo(int bar) { + // ... + } + ``` + </td><td> + + Foo.h: + ```cpp + void foo(int); + ``` + + Foo.cpp: + ```cpp + void foo(int bar) { + // ... + } + ``` + </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 @@ -394,6 +861,32 @@ that you can click on to open them. resolving merge conflicts when multiple sources were added by different people to the same CMakeLists.txt easier. +## GoogleTest-specific + +- Unit tests are not *required* to follow all code standards +- <details><summary> + Private/protected members may be accessed using preprocessor tricks + </summary> + + ```cpp + // include unrelated headers before + + #define private public + #define protected public + + // headers included after *will* be affected + ``` + </details> +- Each test source file defines tests within a single test suite (first + 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 - Files are placed in the appropriate directory: @@ -412,6 +905,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. @@ -436,6 +931,113 @@ that you can click on to open them. void foo(); ``` </td></tr></table></details> +- <details><summary> + The default constructor and destructor aren't required to have a + <code>\brief</code> description + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + Foo(); + virtual ~Foo(); + ``` + </td><td> + + ```cpp + //! Create instance of Foo + Foo(); + //! Destroy instance of Foo + virtual ~Foo(); + ``` + </td></tr></table></details> +- <details><summary> + Parameter direction shouldn't be specified using Doxygen + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + /** + * \param bar Reference to Bar + */ + void foo(const Bar & bar); + ``` + </td><td> + + ```cpp + /** + * \param[in] bar Reference to Bar + */ + void foo(const Bar & bar); + ``` + </td></tr></table></details> +- <details><summary> + Deleted functions shouldn't have Doxygen comments + </summary><table><tr><th>Good</th><th>Bad</th></tr><tr><td> + + ```cpp + // singleton + Foo(const Foo &) = delete; + Foo(Foo &&) = delete; + Foo & operator=(const Foo &) = delete; + Foo & operator=(Foo &&) = delete; + ``` + </td><td> + + ```cpp + //! Deleted copy constructor + Foo(const Foo &) = delete; + //! Deleted move constructor + Foo(Foo &&) = delete; + //! Deleted copy assignment operator + Foo & operator=(const Foo &) = delete; + //! Deleted move assignment operator + 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 @@ -443,4 +1045,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 |