|
| 1 | +# Contributing |
| 2 | + |
| 3 | +## Building |
| 4 | + |
| 5 | +Requires CMake 3.24+, Ninja, and a C++20 compiler (GCC 14 or Clang 18 upward). |
| 6 | +Dependencies are fetched by CMake; there is nothing to install first. |
| 7 | + |
| 8 | +```bash |
| 9 | +cmake --preset debug |
| 10 | +cmake --build --preset debug |
| 11 | +ctest --preset debug --output-on-failure |
| 12 | +``` |
| 13 | + |
| 14 | +Configure and build presets: `debug`, `release`, `asan`, `tsan`, `tidy`. Test |
| 15 | +presets: `debug`, `release`, `asan`, `tsan` — `tidy` has none, because it |
| 16 | +analyses while compiling rather than at test time. Each matches a CI job, so a |
| 17 | +preset passing locally means that job passes. |
| 18 | + |
| 19 | +Two caveats worth knowing before you spend an afternoon on them: |
| 20 | + |
| 21 | +- **TSan excludes the allocation tests.** They replace global `operator new`, |
| 22 | + which is exactly what TSan's runtime also does. `MOTIONKIT_BUILD_ALLOCATION_TESTS` |
| 23 | + is off under that preset, so the count is 128 rather than 139. |
| 24 | +- **`ScurveProfile` is rest-to-rest.** Planning to a position from a non-zero |
| 25 | + velocity is WP-12 and not implemented. `StopProfile` does start from an |
| 26 | + arbitrary state, which is a different problem. |
| 27 | + |
| 28 | +## Before opening a pull request |
| 29 | + |
| 30 | +```bash |
| 31 | +bash scripts/format.sh # or --check to only report |
| 32 | +cmake --preset tidy && cmake --build --preset tidy |
| 33 | +``` |
| 34 | + |
| 35 | +The `tidy` preset sets `CMAKE_CXX_CLANG_TIDY`, so the analysis runs as part of |
| 36 | +compiling; there is no separate test step for it. |
| 37 | + |
| 38 | +CI runs GCC and Clang in Debug and Release, ASan/UBSan, TSan, clang-tidy, |
| 39 | +clang-format, an installed-package consumer build, and the documentation gate. |
| 40 | +All are required. |
| 41 | + |
| 42 | +## The documentation gate |
| 43 | + |
| 44 | +Every public entity needs a doc comment. Adding a public function without one |
| 45 | +fails CI — see [ADR-0009](docs/adr/0009-the-api-reference-is-a-gate.md). |
| 46 | + |
| 47 | +```bash |
| 48 | +cmake -S . -B build-docs -DMOTIONKIT_BUILD_DOCS=ON |
| 49 | +cmake --build build-docs --target docs |
| 50 | +``` |
| 51 | + |
| 52 | +Doxygen is not needed for an ordinary build; the option defaults to `OFF`. |
| 53 | + |
| 54 | +What the gate asks for is a **sentence saying what the entity is for**. It does |
| 55 | +not require `@param` and `@return` on everything, deliberately: `@return The |
| 56 | +size.` is not documentation. Use `@param` where the parameter carries a unit, a |
| 57 | +frame, or an ownership transfer. |
| 58 | + |
| 59 | +One trap the gate exists to catch: a comment binds to **one** entity. This |
| 60 | +documents `lower` and leaves `upper` blank in the published reference, even |
| 61 | +though it reads as covering both: |
| 62 | + |
| 63 | +```cpp |
| 64 | +/// Travel limits in radians. |
| 65 | +Scalar lower{-6.28}; |
| 66 | +Scalar upper{6.28}; |
| 67 | +``` |
| 68 | +
|
| 69 | +## Style |
| 70 | +
|
| 71 | +`.clang-format` (Google, 90 columns) and `.clang-tidy` are authoritative — run |
| 72 | +them rather than reading this section. Conventions they cannot express: |
| 73 | +
|
| 74 | +- **Frames are named `A_T_B`**, read as "the pose of B expressed in A", so that |
| 75 | + `A_T_B * B_T_C` visibly cancels. A composition that does not cancel is a bug |
| 76 | + the reader can see. |
| 77 | +- **Units are SI** — metres, radians, seconds — and are never converted |
| 78 | + silently. Say the unit in the doc comment where a number has one. |
| 79 | +- **Failures a control loop can expect are `Expected<T, E>` values, not |
| 80 | + exceptions.** Exceptions are for a caller who has already broken a |
| 81 | + precondition, such as normalising a zero vector. |
| 82 | +- **Anything callable from the cyclic task allocates nothing and does not |
| 83 | + throw**, and there is a test in `tests/test_no_allocation.cpp` asserting it. |
| 84 | + If you add such a function, add the test too. |
| 85 | +- **Dependencies point down.** See the component diagram and the rules in |
| 86 | + [docs/architecture.md](docs/architecture.md). Siblings must not include each |
| 87 | + other. |
| 88 | +
|
| 89 | +## Tests |
| 90 | +
|
| 91 | +Property tests over generated inputs are preferred to hand-picked values where |
| 92 | +the property is the real claim — that `inverse()` undoes `forward()`, that |
| 93 | +`matrix()` is always in SO(3). Assert the property, not one case of it. |
| 94 | +
|
| 95 | +Two specific habits: |
| 96 | +
|
| 97 | +- **Test the singular case explicitly.** Most of the interesting failures in |
| 98 | + this library live at a singularity, a wrap-around, or a zero-length input. |
| 99 | +- **Give a negative control to any test that could pass by being inert.** The |
| 100 | + allocation counter has one — `TheAllocationCounterItselfWorks` — because a |
| 101 | + counter that never increments passes every test silently. |
| 102 | +
|
| 103 | +## Commits and ADRs |
| 104 | +
|
| 105 | +Commit messages explain **why**, in prose. The diff already shows what changed. |
| 106 | +
|
| 107 | +A decision that a future reader would otherwise have to reverse-engineer goes |
| 108 | +in an ADR under `docs/adr/`, numbered sequentially. Rejected alternatives are |
| 109 | +part of the record: the value is in knowing that an option was considered and |
| 110 | +why it lost, which is the question that gets asked eighteen months later. |
| 111 | +
|
| 112 | +Add an entry to [CHANGELOG.md](CHANGELOG.md) under `Unreleased` for anything |
| 113 | +that changes the public API or observable behaviour. |
0 commit comments