diff --git a/.dir-locals.el b/.dir-locals.el new file mode 100644 index 0000000..a1d1a58 --- /dev/null +++ b/.dir-locals.el @@ -0,0 +1,16 @@ +;;; Directory Local Variables -*- no-byte-compile: t -*- +;;; See AGENTS.md -> "Coding Style" for the rationale. +;;; +;;; The canonical style is cc-mode "stroustrup" with tabs enabled: 4 columns per +;;; level, tabs 8 columns wide, so depth 1 is four spaces, depth 2 is one tab, +;;; depth 3 is a tab plus four spaces. A correctly formatted file is a fixed +;;; point of `indent-region' under these settings. +;;; +;;; Reindent a whole file with C-x h C-M-\, or the tree with +;;; `scripts/reindent.sh'. + +((c-mode . ((c-file-style . "stroustrup") + (indent-tabs-mode . t) + (tab-width . 8) + (fill-column . 100) + (require-final-newline . t)))) diff --git a/AGENTS.md b/AGENTS.md index df3ee9f..3d8c20a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,7 +2,43 @@ ## Project Structure & Module Organization -This is a C library intended to support the development of video games. Public C headers live in `include/akgl/`; keep declarations there aligned with their implementations in `src/`. Tests are standalone C programs under `tests/`, with JSON, image, and map fixtures in `tests/assets/`. The `util/` directory contains the `charviewer` utility and its sample assets. Third-party and companion libraries are vendored in `deps/`. Treat `build/`, generated `akgl.pc` files, and `include/akgl/SDL_GameControllerDB.h` as build outputs rather than hand-maintained source. +This is a C library intended to support the development of video games. Public C headers live in `include/akgl/`; keep declarations there aligned with their implementations in `src/`. Tests are standalone C programs under `tests/`, with JSON, image, and map fixtures in `tests/assets/`. The `util/` directory contains the `charviewer` utility and its sample assets. Third-party and companion libraries are vendored in `deps/`. Treat `build/` and generated `akgl.pc` files as build outputs rather than hand-maintained source; `include/akgl/SDL_GameControllerDB.h` is generated too, but is tracked on purpose — see below. + +## Generated and Vendored Sources + +### `include/akgl/SDL_GameControllerDB.h` is generated, and is tracked deliberately + +`mkcontrollermappings.sh` regenerates this header by fetching the community +controller database from `raw.githubusercontent.com`. **It is committed to the +repository on purpose**: it is the offline fallback that keeps the library +buildable if upstream is renamed, rate-limited, taken down, or simply +unreachable from the build machine. Do not delete it, do not add it to +`.gitignore`, and do not "clean up" the fact that a generated file is tracked. + +Working rules: + +- **Never hand-edit it.** It is machine-written; changes belong in + `mkcontrollermappings.sh`. +- **Do not commit incidental regeneration churn.** The build regenerates the + header on every run, and the script stamps `$(date)` into a comment, so an + ordinary build leaves the file modified with nothing of substance changed. + Revert that before committing: `git checkout -- include/akgl/SDL_GameControllerDB.h`. +- **Update it as a deliberate, standalone commit** when you actually want newer + mappings, so the diff is reviewable and bisectable. +- **Never commit a copy with `AKGL_SDL_GAMECONTROLLER_DB_LEN 0`.** That is the + signature of a failed fetch, not an empty upstream — see the defect note + below. Check the constant before staging this file. +- Formatting tooling ignores it: `scripts/reindent.sh` and the pre-commit hook + both skip it. + +> **Known defect (tracked in `TODO.md`).** The safety net is currently +> self-defeating. `mkcontrollermappings.sh` has no `set -e` and does not check +> `curl`'s exit status, so when the fetch fails it writes a header with +> `AKGL_SDL_GAMECONTROLLER_DB_LEN 0` and an empty array **over the good tracked +> copy, and exits 0**. Because CMake re-runs the generator on every build, an +> offline build silently destroys the very fallback the file exists to provide. +> Until that is fixed, treat a dirty `SDL_GameControllerDB.h` after a build as +> suspect and check the length constant before committing it. ## Build, Test, and Development Commands @@ -30,9 +66,226 @@ Run mutation testing with `cmake --build build --target mutation`. For a quick s Generate HTML and Cobertura coverage reports with `cmake -S . -B build-coverage -DAKGL_COVERAGE=ON -DCMAKE_BUILD_TYPE=Debug`, then build and run CTest. Reports are written to `build-coverage/coverage/`; this mode requires `gcovr` and GCC or Clang. -## Coding Style & Naming Conventions +## Coding Style -Follow the surrounding C style: braces on the next line for function bodies, spaces inside control-flow parentheses, and short, focused functions. Preserve the indentation of the file being edited; older files contain both spaces and tabs. Public symbols use the `akgl_` prefix, public types use `akgl_TypeName`, and constants/macros use `AKGL_UPPER_SNAKE_CASE`. Name matching header/source pairs by feature, such as `include/akgl/sprite.h` and `src/sprite.c`. No repository-wide formatter or linter is configured, so avoid unrelated formatting churn. +The canonical style is Emacs `cc-mode` **`stroustrup`**, with tabs enabled. This +is not advisory — new and edited code must match what `cc-mode` produces, so that +reindenting a region never shows up as a diff. + +### Emacs setup + +`.dir-locals.el` in the repository root already applies the style to every +`c-mode` buffer, so nothing needs configuring by hand: + +```elisp +((c-mode . ((c-file-style . "stroustrup") + (indent-tabs-mode . t) + (tab-width . 8) + (fill-column . 100) + (require-final-newline . t)))) +``` + +Interactively: `C-x h C-M-\` (`mark-whole-buffer` + `indent-region`) reindents +the buffer. A correctly formatted file is a fixed point of that operation — +reindenting must produce no change. + +### Reindenting from the command line + +```sh +scripts/reindent.sh # reindent every tracked C source in place +scripts/reindent.sh src/tilemap.c # reindent only the named files +scripts/reindent.sh --check # list non-conforming files, change nothing +``` + +`--check` exits 1 when something needs reindenting and 2 if Emacs is missing, so +it is usable from CI. The script drives Emacs in batch mode through +`scripts/reindent.el`, which reindents, normalises leading whitespace to the +canonical tab/space mix, strips trailing whitespace, and ensures a final +newline. `include/akgl/SDL_GameControllerDB.h` is generated and always skipped. + +Note that `reindent.el` deliberately does **not** use Emacs' `tabify`: that +function's `tabify-regexp` is `" [ \t]+"`, which is not anchored to the start of +a line, so it rewrites runs of spaces anywhere and destroys the hand-aligned +value columns in the bit-flag tables in `actor.h` and `iterator.h`. Only leading +whitespace is ever rewritten. + +### The pre-commit hook + +`scripts/hooks/pre-commit` reindents staged C sources before the commit is +written. Enable it once per clone: + +```sh +git config core.hooksPath scripts/hooks +``` + +It inspects the *staged* content rather than the working tree, so what lands in +the commit is what was checked. When a file needs reindenting it is fixed in the +working tree and re-staged — but only if the index and working tree agree for +that file. If they differ, re-staging would sweep unstaged work into the commit, +so the hook stops and tells you to reindent and stage it yourself. It steps +aside during a merge, and warns rather than blocking if Emacs is unavailable. +`git commit --no-verify` bypasses it. + +For reference, `cc-mode` defines the style as: + +```elisp +("stroustrup" + (c-basic-offset . 4) + (c-comment-only-line-offset . 0) + (c-offsets-alist . ((statement-block-intro . +) + (substatement-open . 0) + (substatement-label . 0) + (label . 0) + (statement-cont . +)))) +``` + +### Indentation and whitespace + +- **4 columns per level** (`c-basic-offset` 4). +- **Tabs are 8 columns wide and are used for indentation** (`indent-tabs-mode` + `t`, `tab-width` 8). Emacs emits the largest possible run of tabs and pads the + remainder with spaces, which produces this ladder. Editors that expand tabs, or + that assume a 4-column tab, will silently corrupt it: + + | Depth | Columns | Bytes | + |---|---|---| + | 1 | 4 | 4 spaces | + | 2 | 8 | `TAB` | + | 3 | 12 | `TAB` + 4 spaces | + | 4 | 16 | `TAB` `TAB` | + | 5 | 20 | `TAB` `TAB` + 4 spaces | + +- No trailing whitespace. Files end with a single newline. +- `case` labels sit at the same column as their `switch` (`label . 0`). +- Continuation lines of a wrapped expression indent one level (`statement-cont . +`). + +Most of `src/` already conforms. The known non-conforming files are +`src/json_helpers.c`, `src/util.c` (from `akgl_rectangle_points` onward), +`src/assets.c`, `src/staticstring.c`, parts of `src/actor.c`, and the headers +`include/akgl/util.h` and `include/akgl/staticstring.h` — all of which use a +2-column offset. Convert a file to the canonical style in its own commit, not +mixed into a behavioral change. + +### Braces + +- **Function bodies open on their own line, in column 0.** +- **Control statements keep the brace on the same line**, one space before it. +- **`else`, `else if`, and `while` of a `do`/`while` stay on the closing-brace + line**: `} else {`, not a bare `else` on the next line. Note that this is a + house convention rather than something `cc-mode` enforces — the `stroustrup` + style governs indentation only and will not move a brace or an `else` for you. +- **Always brace, even single-statement bodies.** + +```c +akerr_ErrorContext *akgl_actor_update(akgl_Actor *obj) +{ + if ( obj->curSpriteFrameId == 0 ) { + obj->curSpriteReversing = false; + } else if ( obj->parent != NULL ) { + obj->curSpriteFrameId -= 1; + } else { + obj->curSpriteFrameId += 1; + } +} +``` + +### Spacing + +- **Spaces inside control-flow parentheses**: `if ( x == y ) {`, `while ( done == false ) {`, + `for ( i = 0; i < len; i++ ) {`. This is the dominant existing convention + (140 sites to 7) and `cc-mode` will not add or remove it. +- No space between a function name and its argument list, at both call and + definition sites, and no padding inside call parentheses: `SDL_Log("x %d", n)`. +- Binary operators and assignment are surrounded by single spaces; unary + operators are not separated from their operand. +- **The pointer `*` binds to the identifier**, not the type: `char *name`, + `akgl_Actor **dest`. Never `char* name`. +- When a call is too long for one line, put each argument on its own line + indented one level past the callee, with the closing `)` on its own line. This + is the established shape for the `FAIL_*` and `CATCH` macros: + + ```c + FAIL_ZERO_RETURN( + errctx, + SDL_SetPointerProperty(AKGL_REGISTRY_ACTOR, name, (void *)obj), + AKERR_KEY, + "Unable to add actor to registry" + ); + ``` + +### No repository-wide formatter + +There is no `clang-format` or linter wired into the build, and `cc-mode`'s +indentation engine is not exactly reproducible by `clang-format`. Do not +introduce one without discussion, and do not reformat code you are not otherwise +changing — unrelated whitespace churn makes review harder and is the reason the +style drifted in the first place. + +## Naming Conventions + +- **Public functions**: `akgl__`, all lower snake_case — + `akgl_actor_set_character`, `akgl_heap_next_string`. No camelCase, and never + embed a type name (`akgl_Actor_cmhf_left_on` is wrong; it should be + `akgl_actor_cmhf_left_on`). +- **Public types**: `akgl_TypeName` — `akgl_Actor`, `akgl_SpriteSheet`, + `akgl_PhysicsBackend`. Every type exported from a header takes the prefix; + bare names like `point` and `RectanglePoints` are defects, not precedent. +- **Constants and macros**: `AKGL_UPPER_SNAKE_CASE`. A constant belongs to the + subsystem it describes: a character limit is `AKGL_CHARACTER_MAX_*`, not + `AKGL_SPRITE_MAX_CHARACTER_*`. Name a constant for what its value *is* — a + nanoseconds-per-millisecond scale factor is `AKGL_TIME_ONEMS_NS`. +- **Exported globals** take the `akgl_` prefix like any other public symbol. Do + not add bare names (`renderer`, `camera`, `window`), leading-underscore names + (reserved at file scope), or SCREAMING_SNAKE names for mutable objects — + `AKGL_UPPER_SNAKE_CASE` is for constants. +- **`static` helpers drop the `akgl_` prefix**, which exists only to avoid + external collisions. +- **Include guards**: `_AKGL__H_`, matching the file name. Guard names + without the project prefix risk colliding with system headers. +- **Parameter names must match between the declaration and the definition** — + Doxygen publishes the header spelling. Conventional names: `dest` for an + output parameter (not `dst`), `self` for a backend receiver, `obj` for the + instance being initialized or inspected, `e` for an incoming error context to + be inspected. +- **The local error context is named `errctx`** (92 sites to 45). New code uses + `errctx`; convert `e` when you touch a function for other reasons. +- **Header/source pairs are named by feature**: `include/akgl/sprite.h` and + `src/sprite.c`. + +## Error-Handling Protocol + +The `akerror` control-flow macros have a shape that must be followed exactly, +because the failure mode is silent. + +- Inside an `ATTEMPT` block use the **`_BREAK`** variants (`FAIL_ZERO_BREAK`, + `FAIL_NONZERO_BREAK`, `FAIL_BREAK`) and `CATCH`. Outside it use the + **`_RETURN`** variants. +- **Never use a `*_RETURN` macro inside an `ATTEMPT` block.** It returns past the + `CLEANUP` block, so every release, `fclose`, and free in `CLEANUP` is skipped. + This has already caused a heap-string leak on the success path of + `akgl_get_json_tilemap_property`. +- `CLEANUP` must precede `PROCESS`. Transposing them moves the cleanup body into + the `PROCESS` switch, where it runs only when an error context exists. +- `CATCH` reports failure by `break`ing, which binds to the innermost enclosing + loop or `switch`. A `CATCH` written directly inside a `while` exits the loop + rather than the function — put the `ATTEMPT` block inside the loop. +- Validate every pointer parameter before dereferencing it, including the ones a + sibling function happens not to check. + +## API Surface + +Every function with external linkage must be declared in a header, and every +declaration must have a definition. A non-`static` function that appears in no +header is still in the ABI but is unreachable by callers; a declaration with no +definition is a link error for anyone compiling against the header alone. If a +function is exposed only so tests can reach it, declare it under the existing +"part of the internal API" comment block in the relevant header. + +Headers must be self-contained: include what you use, so that a translation unit +including a single `akgl` header compiles without a particular include order. +Within the project use the angled form, `#include `. + +Declare no-argument functions as `(void)`, not `()`. ## Rules diff --git a/scripts/hooks/pre-commit b/scripts/hooks/pre-commit new file mode 100755 index 0000000..163a964 --- /dev/null +++ b/scripts/hooks/pre-commit @@ -0,0 +1,98 @@ +#!/bin/sh +# +# Reindent staged C sources to the canonical style before the commit is made. +# See AGENTS.md -> "Coding Style". +# +# Enable with: +# git config core.hooksPath scripts/hooks +# Bypass a single commit with `git commit --no-verify`. +# +# The hook checks the *staged* content, not the working tree, so what gets +# committed is what was verified. When a file needs reindenting it is fixed in +# the working tree and re-staged -- but only when the working tree and the index +# agree for that file. If they disagree (a partial `git add -p`), re-staging +# would sweep unstaged work into the commit, so the hook stops and asks you to +# do it yourself. + +set -eu + +root=$(git rev-parse --show-toplevel) +reindent="$root/scripts/reindent.sh" + +# Leave conflict resolution alone. +if [ -e "$root/.git/MERGE_HEAD" ]; then + exit 0 +fi + +if [ ! -x "$reindent" ]; then + echo "pre-commit: $reindent missing or not executable; skipping style check" >&2 + exit 0 +fi + +# Without Emacs the canonical style cannot be applied or even checked. Warn +# rather than block: a hook that fails closed on a missing optional tool just +# teaches everyone to pass --no-verify. +if ! command -v emacs >/dev/null 2>&1; then + echo "pre-commit: emacs not found; skipping reindent (see AGENTS.md)" >&2 + exit 0 +fi + +staged=$(git diff --cached --name-only --diff-filter=ACMR \ + | grep -E '^(src|include|tests|util)/.*\.[ch]$' \ + | grep -v -x -F 'include/akgl/SDL_GameControllerDB.h' || true) + +if [ -z "$staged" ]; then + exit 0 +fi + +tmp=$(mktemp -d) +trap 'rm -rf "$tmp"' EXIT INT TERM + +# Reindent a copy of each file's staged content and see whether it moves. +needs='' +for f in $staged; do + copy="$tmp/$(printf '%s' "$f" | tr '/' '_')" + git show ":$f" >"$copy" + cp "$copy" "$copy.orig" + if ! "$reindent" "$copy" >/dev/null 2>&1; then + echo "pre-commit: reindent failed on $f; commit aborted" >&2 + exit 1 + fi + if ! cmp -s "$copy" "$copy.orig"; then + needs="$needs $f" + fi +done + +if [ -z "$needs" ]; then + exit 0 +fi + +# Refuse to touch anything if even one file is partially staged. +blocked='' +for f in $needs; do + if ! git diff --quiet -- "$f"; then + blocked="$blocked $f" + fi +done + +if [ -n "$blocked" ]; then + echo "pre-commit: these files need reindenting but have unstaged changes," >&2 + echo "so re-staging them would pull unstaged work into the commit:" >&2 + for f in $blocked; do echo " $f" >&2; done + echo >&2 + echo "Reindent and stage them yourself, then commit again:" >&2 + echo " scripts/reindent.sh$(for f in $blocked; do printf ' %s' "$f"; done)" >&2 + echo " git add$(for f in $blocked; do printf ' %s' "$f"; done)" >&2 + exit 1 +fi + +# Safe: for every file needing work, the index and working tree agree. +# shellcheck disable=SC2086 +"$reindent" $needs +# shellcheck disable=SC2086 +git add $needs + +echo "pre-commit: reindented and re-staged:" >&2 +for f in $needs; do echo " $f" >&2; done + +exit 0 diff --git a/scripts/reindent.el b/scripts/reindent.el new file mode 100644 index 0000000..a82c1d1 --- /dev/null +++ b/scripts/reindent.el @@ -0,0 +1,87 @@ +;;; reindent.el --- batch reindent to cc-mode "stroustrup" -*- lexical-binding: t -*- + +;; Reindents each file named on the command line, in place, to the project's +;; canonical style: cc-mode "stroustrup", 4 columns per level, tabs 8 columns +;; wide. See AGENTS.md -> "Coding Style". +;; +;; The style is set explicitly here rather than read from .dir-locals.el so that +;; the result does not depend on where the file lives -- the pre-commit hook +;; runs this over temporary copies outside the project tree. +;; +;; Usage: emacs --batch -Q -l scripts/reindent.el -- FILE... + +;;; Code: + +(require 'cc-mode) + +(setq make-backup-files nil + create-lockfiles nil + auto-save-default nil + vc-handled-backends nil + inhibit-message t) + +(defun akgl-retab-leading-whitespace () + "Rewrite every line's leading whitespace as tabs-then-spaces at `tab-width'. + +`indent-region' fixes the column a line starts at, but `indent-line-to' leaves +a line alone when it is already at the right column even if the bytes are +wrong -- eight spaces where the canonical form is one tab. This pass closes +that gap. + +Deliberately not `tabify': that function's `tabify-regexp' is \" [ \\t]+\", +which is NOT anchored to the line start, so it rewrites runs of spaces +anywhere on the line and destroys the hand-aligned columns in the bit-flag +tables in actor.h and iterator.h. Only leading whitespace is touched here, and +lines beginning inside a string literal are skipped outright." + (goto-char (point-min)) + (while (not (eobp)) + (let ((bol (point))) + (unless (nth 3 (syntax-ppss bol)) + (skip-chars-forward " \t" (line-end-position)) + (let ((col (current-column))) + (unless (or (zerop col) (eolp)) + (let ((want (concat (make-string (/ col tab-width) ?\t) + (make-string (% col tab-width) ?\s)))) + (unless (string= want (buffer-substring bol (point))) + (delete-region bol (point)) + (insert want))))))) + (forward-line 1))) + +(defun akgl-reindent-file (path) + "Reindent PATH in place. Returns t if the file changed on disk." + (let ((before (with-temp-buffer + (insert-file-contents path) + (buffer-string)))) + (with-current-buffer (find-file-noselect path t) + (c-mode) + (c-set-style "stroustrup") + (setq indent-tabs-mode t + tab-width 8 + c-basic-offset 4 + require-final-newline t) + ;; 1. Put every line at its correct column. + (indent-region (point-min) (point-max)) + ;; 2. Convert leading whitespace to the canonical tab/space mix. + (akgl-retab-leading-whitespace) + ;; 3. Trailing whitespace and a single final newline. + (delete-trailing-whitespace) + (goto-char (point-max)) + (unless (bolp) (insert "\n")) + (let ((changed (not (string= before (buffer-string))))) + (when changed (save-buffer)) + (kill-buffer) + changed)))) + +;; Emacs leaves the "--" separator in `command-line-args-left'; drop it, along +;; with any empty argument, so the remainder is exactly the file list. +(dolist (path (seq-remove (lambda (a) (or (string= a "--") (string= a ""))) + command-line-args-left)) + (when (akgl-reindent-file path) + (princ (format "reindented %s\n" path)))) + +;; Exit 0 on success whether or not anything changed, so that any non-zero +;; status from this script means a real failure. Callers detect "something +;; changed" from stdout, or by comparing files themselves. +(kill-emacs 0) + +;;; reindent.el ends here diff --git a/scripts/reindent.sh b/scripts/reindent.sh new file mode 100755 index 0000000..12e34ea --- /dev/null +++ b/scripts/reindent.sh @@ -0,0 +1,92 @@ +#!/bin/sh +# +# Reindent C sources to the project's canonical style (cc-mode "stroustrup", +# 4-column offset, 8-column tabs). See AGENTS.md -> "Coding Style". +# +# scripts/reindent.sh reindent every tracked C source in place +# scripts/reindent.sh FILE... reindent only the named files +# scripts/reindent.sh --check ... report non-conforming files, change nothing +# +# Exit status: 0 if everything already conforms (or was reindented), 1 if +# --check found a file that needs reindenting, 2 on a usage or environment +# error. + +set -eu + +root=$(git rev-parse --show-toplevel) +elisp="$root/scripts/reindent.el" + +# Directories whose C sources are hand-maintained. Anything outside these is +# vendored (deps/) or generated (include/akgl/SDL_GameControllerDB.h) and is +# left alone. +SCOPE='src include tests util' +GENERATED='include/akgl/SDL_GameControllerDB.h' + +check=0 +if [ "${1:-}" = "--check" ]; then + check=1 + shift +fi + +if ! command -v emacs >/dev/null 2>&1; then + echo "reindent: emacs not found; cannot verify or apply the canonical style" >&2 + exit 2 +fi +if [ ! -f "$elisp" ]; then + echo "reindent: missing $elisp" >&2 + exit 2 +fi + +# Build the file list: explicit arguments, or every tracked C source in scope. +if [ "$#" -gt 0 ]; then + files=$(for f in "$@"; do printf '%s\n' "$f"; done) +else + files=$(cd "$root" && git ls-files $SCOPE | grep -E '\.[ch]$' || true) +fi + +# Drop generated files and anything that no longer exists on disk. +files=$(printf '%s\n' "$files" | grep -v -x -F "$GENERATED" || true) +files=$(cd "$root" && for f in $files; do [ -f "$f" ] && printf '%s\n' "$f"; done) + +if [ -z "$files" ]; then + exit 0 +fi + +if [ "$check" -eq 0 ]; then + # Reindent in place. A non-zero status here is a real failure -- never + # swallow it, or a broken Emacs would look like "everything already conforms". + # shellcheck disable=SC2086 + (cd "$root" && emacs --batch -Q -l "$elisp" -- $files) || { + echo "reindent: emacs failed; no files were reindented" >&2 + exit 2 + } + exit 0 +fi + +# --check: reindent throwaway copies and report which originals differ. The +# copies keep their extension so cc-mode still selects the right major mode. +tmp=$(mktemp -d) +trap 'rm -rf "$tmp"' EXIT INT TERM + +copies='' +for f in $files; do + dest="$tmp/$(printf '%s' "$f" | tr '/' '_')" + cp "$root/$f" "$dest" + copies="$copies $dest" +done + +# shellcheck disable=SC2086 +emacs --batch -Q -l "$elisp" -- $copies >/dev/null || { + echo "reindent: emacs failed; cannot determine whether sources conform" >&2 + exit 2 +} + +status=0 +for f in $files; do + dest="$tmp/$(printf '%s' "$f" | tr '/' '_')" + if ! cmp -s "$root/$f" "$dest"; then + echo "$f" + status=1 + fi +done +exit $status