# C++ API and ABI boundaries

Load when designing a public library header, exposing C++ to C, shipping a shared library other people link against, or changing anything already released.

## Exceptions must not cross an `extern "C"` boundary

`extern "C"` sets *language linkage*, not the caller's language: a C++ function with C linkage can still legally throw to a C++ caller. The reason to catch everything anyway is the contract. A C caller has no channel to receive a C++ exception, and unwinding through frames actually compiled as C is toolchain-dependent (GCC documents `-fexceptions` as sometimes necessary for exactly this). So every entry point of a C API converts:

```cpp
/* h and input must be non-NULL. On success *out_err is set to NULL.
 * On failure *out_err receives a message owned by the library; release
 * it with lib_error_free(). */
extern "C" int lib_do_thing(lib_handle *h, const char *input, char **out_err)
{
    if (out_err) *out_err = NULL;
    if (h == NULL || input == NULL)
        return LIB_ERR_ARG;

    try {
        reinterpret_cast<Impl *>(h)->doThing(input);
        return LIB_OK;
    } catch (const std::exception &e) {
        if (out_err) *out_err = lib_dup_message(e.what());
        return LIB_ERR;
    } catch (...) {
        if (out_err) *out_err = lib_dup_message("unknown error");
        return LIB_ERR;
    }
}
```

Three things that sample is doing deliberately. It states the non-NULL contract and enforces it, because a C caller gets no reference types to lean on. It clears `*out_err` on entry, so a caller that inspects it after success reads NULL rather than a stale pointer. And the message crosses the boundary through the library's own `lib_dup_message`/`lib_error_free` pair rather than `strdup`/`free`: the caller may link a different allocator, and whoever allocates must be whoever frees. `lib_dup_message` returning NULL under memory pressure is acceptable; the status code still reports the failure.

The bare `catch (...)` is not optional. A `catch (const std::exception &)` alone still lets an `int`, a string literal, or a foreign exception type escape.

The same rule applies to any callback a C library invokes: the callback body is a C frame boundary, so it catches everything and reports through the C API's error channel. This is the failure mode when wrapping a C++ vendor library from a C extension.

## `extern "C"` signature constraints

Only C-compatible types cross: fundamental types, pointers, and trivially-copyable structs with a stable layout. No references, no templates, no `std::` types, no default arguments, no overloading (there is no mangling to distinguish them).

Hand out an opaque pointer to the C++ object and a matching destroy function. `struct lib_handle;` declared but never defined in the public header prevents callers from dereferencing it.

## What breaks ABI

Once a shared library is released, all of the following break consumers even though they still compile. They break in three different ways, and the category decides how the break shows up.

**Layout breaks** — object size or member offsets are baked into already-compiled callers:

| Change | Why it breaks |
|---|---|
| Adding, removing, or reordering non-static data members | Size and offsets are compiled into callers |
| Adding the *first* virtual function | Adds a vtable pointer: changes size and layout |
| Adding to an existing vtable | Layout- and inheritance-model-dependent. Appending can preserve existing slots under narrow conditions, but never assume it; verify with an ABI diff tool |
| Changing a base class or its order | Shifts subobject offsets |
| Changing an enum's underlying type | Changes size and how it is passed |
| Changing alignment, packing, or a bitfield layout | Same as reordering |

**Symbol breaks** — whether the mangled name changes decides whether the break surfaces at link time:

| Change | Why it breaks |
|---|---|
| Changing parameter types or member-function `const`ness | Mangled name changes, so the old symbol disappears. **Loud**: callers fail to link |
| Changing only the return type | Itanium does not mangle an ordinary function's return type, so the symbol is unchanged and callers keep binding to it while disagreeing about what comes back. **Silent** |
| Adding or removing `noexcept` on a released function | Also absent from an ordinary function's mangled name. Callers compiled against the old spec may have omitted unwind handling. **Silent** |

**Semantic skew** — symbols and layout both survive, so nothing fails until a mixed-version deployment runs:

| Change | Why it breaks |
|---|---|
| Changing the body of an inline function | The old body is already inlined into callers |
| Changing a default argument value | Evaluated at the call site, not in the library |
| Changing an enum's numeric value | Callers embedded the old number |

The silent rows in the last two tables are the dangerous ones. A break that changes a mangled name announces itself; a break that preserves it ships. Run an ABI diff (`abi-compliance-checker`, `abidiff`) against the previous release rather than reasoning row by row.

Safe additions: new non-virtual, non-inline member functions; new free functions; new types; new overloads that do not displace existing ones.

The C-side counterparts, where to consume a new flag bit in a published bitfield and why new state never goes into a released struct's padding, are in the `ia-c-systems` skill.

## PIMPL when ABI stability is required

```cpp
// public header
class Client {
public:
    Client();
    ~Client();
    Client(Client &&) noexcept;
    Client &operator=(Client &&) noexcept;

    void connect(const char *host);

private:
    struct Impl;
    std::unique_ptr<Impl> impl_;
};
```

This hides `Impl`'s layout, so the implementation grows members freely. Be precise about what it does *not* hide: `Client` itself still has a visible layout of exactly one `unique_ptr`, and callers bake in that pointer's size and alignment. PIMPL therefore stabilizes `Client` only within a compatible compiler and standard-library ABI. Note the signature takes `const char *` rather than `const std::string &` for the same reason the section below gives; a `std::string` parameter would put a standard-library type straight into the boundary PIMPL exists to protect. For a genuinely cross-toolchain boundary, PIMPL is not enough: expose an opaque C handle with exported lifetime functions.

Two mechanics that trip people:

- The destructor must be **declared in the header and defined in the `.cpp`**, after `Impl` is complete. A defaulted destructor in the header fails to compile against an incomplete type.
- Move operations must be declared and defined in the `.cpp` for the same reason. Declaring the destructor already suppressed the implicit ones.

Cost: one allocation per object and one indirection per call. Pay it at a stable library boundary, not on an internal type.

## Standard library types in a public ABI

Do not expose `std::string`, `std::vector`, or any other standard container by value or by reference across a shared-library boundary that consumers may build differently. Their layout varies with standard library implementation, `_GLIBCXX_USE_CXX11_ABI`, libstdc++ debug mode (`_GLIBCXX_DEBUG`), MSVC's `_ITERATOR_DEBUG_LEVEL`, and standard version.

How a mismatch surfaces depends on where the type appears. When it is part of a mangled signature, `_GLIBCXX_USE_CXX11_ABI` mismatches are link errors *by design* — that is the entire purpose of the `std::__cxx11` inline namespace and `abi_tag`. The silent case is the one to fear: a standard type embedded in a user struct, or crossing an opaque boundary such as a `void *` or a plugin interface, where nothing forces the mangled names to disagree and the layouts simply differ. Note also that `_GLIBCXX_ASSERTIONS` is libstdc++'s hardening macro and is documented as ABI-neutral; `_GLIBCXX_DEBUG` is the one that changes layout.

At a hard boundary, pass `const char *` plus length, or a trivially-copyable struct the library owns. Inside one build unit, or in a header-only library the consumer compiles with their own flags, the standard types are fine.

Throwing exceptions across a shared library boundary requires consistent RTTI and typeinfo, which means one compiler and one standard library. Never do it across a plugin boundary the host might have built differently.

## Symbol visibility

Default visibility exports every symbol, which bloats the dynamic symbol table, slows load time, and turns internal helpers into an accidental API nobody can change.

```cmake
set_target_properties(mylib PROPERTIES
    CXX_VISIBILITY_PRESET hidden
    VISIBILITY_INLINES_HIDDEN ON)
```

Then mark the public surface explicitly:

```cpp
#if defined(_WIN32)
#  define LIB_API __declspec(dllexport)
#else
#  define LIB_API __attribute__((visibility("default")))
#endif

class LIB_API Client { /* ... */ };
```

Generate this with CMake's `generate_export_header` rather than hand-rolling the import and export halves.

## API evolution that does not break callers

- **Decide `explicit` at introduction.** Adding it later is source-breaking: every call site relying on the implicit conversion stops compiling. Removing it later is safe but widens the API permanently.
- **Keep an overload when adding a better one.** Delegate the old to the new. Deleting an overload breaks callers even when a superset exists.
- **Delete rather than silently ignore.** When an overload cannot honor part of an argument's state, `= delete` it or `static_assert`, so the caller learns at compile time instead of filing a bug that the feature does nothing.
- **`[[deprecated("use X instead")]]`** for one release before removal, and only where a replacement exists.
- **Inline namespaces** (`inline namespace v2 { ... }`) version an entire API surface within one library, so old and new symbols coexist and mismatches are link errors rather than corruption.

## Verify

- Every `extern "C"` entry point has a `catch (...)`
- No `std::` type crosses a shared-library boundary consumers build themselves
- PIMPL destructor and move operations defined in the `.cpp`, not defaulted in the header
- Visibility is hidden by default with an explicit export macro on the public surface
- For a released library: `abi-compliance-checker` or an equivalent diff against the previous release shows no incompatible change

# CMake and C++ tooling

Load when setting up or reviewing a C++ build, adding a dependency, wiring sanitizers, or configuring static analysis.

## Target-based CMake

Modern CMake describes targets and their requirements. Directory-scoped commands leak settings into everything below them and are the root of most "works in my build" reports.

| Never | Use instead |
|---|---|
| `include_directories(...)` | `target_include_directories(tgt PUBLIC ...)` |
| `add_definitions(-DFOO)` | `target_compile_definitions(tgt PRIVATE FOO)` |
| appending to `CMAKE_CXX_FLAGS` | `target_compile_options(tgt PRIVATE ...)` |
| `link_directories(...)` | `target_link_libraries(tgt PRIVATE Ns::dep)` |
| `set(CMAKE_CXX_FLAGS "-std=c++17")` | `target_compile_features(tgt PUBLIC cxx_std_17)` |

`PRIVATE` means "I need this to build"; `INTERFACE` means "my consumers need this"; `PUBLIC` is both. Getting these wrong is how an implementation detail becomes part of a library's contract.

```cmake
cmake_minimum_required(VERSION 3.20)
project(mylib LANGUAGES CXX)

find_package(OpenSSL REQUIRED)

add_library(mylib src/client.cpp src/query.cpp)
add_library(Mylib::mylib ALIAS mylib)

target_compile_features(mylib PUBLIC cxx_std_17)
target_include_directories(mylib
    PUBLIC  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
            $<INSTALL_INTERFACE:include>
    PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src)
target_link_libraries(mylib PRIVATE OpenSSL::SSL)
```

The generator expressions matter, and they fail loudly rather than subtly: a bare absolute source path in a `PUBLIC` `target_include_directories` makes `install(EXPORT ...)` hard-error with "INTERFACE_INCLUDE_DIRECTORIES property contains path ... which is prefixed in the source directory".

Always export `compile_commands.json`, since clang-tidy, clangd, and IWYU all consume it:

```cmake
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
```

## Warnings as an interface target

Define the warning set once and link it into every target, rather than repeating flags:

```cmake
add_library(project_warnings INTERFACE)
target_compile_options(project_warnings INTERFACE
    "$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wall;-Wextra;-Wpedantic;-Wshadow;-Wconversion;-Wnon-virtual-dtor;-Wold-style-cast;-Wcast-align>"
    "$<$<CXX_COMPILER_ID:MSVC>:/W4;/permissive->")

target_link_libraries(mylib PRIVATE project_warnings)
```

The quoted, semicolon-separated spelling is the canonical way to carry a flag *list* through a generator expression, and it stays unambiguous wherever the surrounding context handles whitespace differently. (An unquoted, space-separated genex also works in `target_compile_options` and resolves to the same list; the quoted form is the one to reach for by default, not a bug fix.)

Add `-Werror` in CI only. Making it the local default turns every new compiler version into a blocked workday.

## Presets

`CMakePresets.json` replaces the wiki page of build incantations and gives CI and humans the same commands:

```json
{
  "version": 2,
  "configurePresets": [
    {
      "name": "dev",
      "generator": "Ninja",
      "binaryDir": "build/dev",
      "cacheVariables": {
        "CMAKE_BUILD_TYPE": "Debug",
        "CMAKE_EXPORT_COMPILE_COMMANDS": "ON"
      }
    },
    {
      "name": "asan",
      "inherits": "dev",
      "binaryDir": "build/asan",
      "cacheVariables": {
        "CMAKE_CXX_FLAGS": "-fsanitize=address,undefined -fno-omit-frame-pointer -g -O1"
      }
    }
  ],
  "buildPresets": [
    { "name": "dev",  "configurePreset": "dev" },
    { "name": "asan", "configurePreset": "asan" }
  ],
  "testPresets": [
    {
      "name": "asan",
      "configurePreset": "asan",
      "output": { "outputOnFailure": true }
    }
  ]
}
```

`cmake --preset asan && cmake --build --preset asan && ctest --preset asan`.

Two things that bite here. `cmake --build --preset` and `ctest --preset` read `buildPresets` and `testPresets`; a file carrying only `configurePresets` fails with "no such preset" on both. And the schema `version` sets a CMake floor of its own (2 needs 3.20, 3 needs 3.21, 6 needs 3.25) which must not exceed the project's `cmake_minimum_required`.

Sanitizer notes: ASan, TSan, and MSan are mutually exclusive, so each needs its own build directory. UBSan composes with any one of them (`address,undefined`, `thread,undefined`, and `memory,undefined` all link). Set `UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=1`, because UBSan otherwise prints diagnostics into a passing test run.

## Dependencies

| Mechanism | When |
|---|---|
| `find_package(Foo REQUIRED)` | The dependency is expected on the system or from a package manager. Default for anything widely packaged. |
| `FetchContent` | Small, source-buildable dependency with no system packaging. Pin a tag or commit, never a branch. |
| Git submodule | The dependency needs local patches, or the build must work fully offline. |

Always consume through a namespaced imported target (`OpenSSL::SSL`, `GTest::gtest_main`). A bare `${FOO_LIBRARIES}` variable carries no include directories, no compile definitions, and no transitive requirements.

## Testing with CTest

```cmake
include(CTest)
find_package(GTest REQUIRED)

add_executable(mylib_ut ut/client_ut.cpp ut/query_ut.cpp)
target_link_libraries(mylib_ut PRIVATE mylib GTest::gtest_main project_warnings)

include(GoogleTest)
gtest_discover_tests(mylib_ut)
```

`gtest_discover_tests` registers each gtest case with CTest individually, so `ctest -R Query` selects real cases and a failure names the case rather than the binary. `add_test` on the whole executable gives one opaque pass/fail.

## Static analysis

`.clang-tidy` at the repo root, applied to changed files rather than the whole tree at first:

```yaml
Checks: >
  bugprone-*,
  cert-*,
  cppcoreguidelines-*,
  modernize-*,
  performance-*,
  readability-*,
  -modernize-use-trailing-return-type,
  -readability-magic-numbers
WarningsAsErrors: 'bugprone-*,cert-*'
HeaderFilterRegex: '^(include|src)/'
```

`HeaderFilterRegex` is required, or clang-tidy either ignores headers entirely or floods the output with findings from system and third-party headers.

`clang-analyzer-*` is absent from that `Checks:` list on purpose -- clang-tidy enables it by default and a config-file `Checks:` adds to the default set rather than replacing it, so the path-sensitive checks (use-after-free, uninitialized reads) run either way. Verify on the local toolchain with `clang-tidy --list-checks` if in doubt. What the list above *does* decide is `WarningsAsErrors`: `bugprone-*,cert-*` are fatal and analyzer findings are not, so a use-after-free warns and CI still goes green. Promoting `clang-analyzer-*` to fatal is defensible on a clean tree and hostile on a legacy one, since path-sensitive analysis has a real false-positive rate -- decide it deliberately and write the decision down, rather than inheriting the warn-only behavior by omission.

Run on a diff rather than the tree: `git diff -U0 origin/main | clang-tidy-diff.py -p1 -path build/dev`. Enabling the full check set on a legacy tree produces thousands of findings and gets the tool switched off.

`.clang-format` is the repo's, not a personal preference. Enforce with `clang-format --dry-run --Werror` in CI, and format only the changed lines (`git-clang-format`) so a formatting sweep never hides a logic change in the same commit.

## Cross-platform, before the first Windows CI run

A codebase that has only ever built on Linux hits the same sequence on its first MSVC lane. None are deep; each costs a CI cycle.

- **`NOMINMAX` before any Windows header.** `windows.h` defines `min` and `max` as macros, which breaks every `std::min`/`std::max` call and produces baffling errors inside templated headers (`error C2589: '(': illegal token on right side of '::'`). Set it project-wide as a compile definition, not per-file.
- **`/bigobj` on heavily templated code.** A deep template hierarchy generates enough sections per translation unit to exceed MSVC's default object-section limit on debug builds. Cheaper to add up front than to diagnose later.
- **POSIX functions MSVC does not have.** `timegm` is the common one; Microsoft's documented equivalent is `_mkgmtime` with identical semantics. Never substitute `mktime` as a fallback: it interprets the `tm` as **local** time and silently shifts every result by the runner's timezone offset, which produces wrong timestamps rather than a build error.
- **Object-handler and callback function pointers.** MSVC warns on incompatible function-pointer assignment (C4133) where GCC is silent, so a signature mismatch that has always been latent surfaces only on the Windows lane.

Run the widest warning set on the platform that is *not* used for daily development. Each compiler is silent about a different class, so a second toolchain in CI is a second static analyser for free.

## Build speed

- `ccache` (or `sccache`), wired in with `set(CMAKE_CXX_COMPILER_LAUNCHER ccache)`.
- Ninja over Make.
- `include-what-you-use` to cut transitive-include creep; a header that pulls in half the standard library slows every consumer.
- Precompiled headers (`target_precompile_headers`) for genuinely stable, widely-included sets only. On a churning header they make builds slower.
- Unity builds (`CMAKE_UNITY_BUILD`) can halve a full build and will break on anonymous-namespace and macro collisions. Verify the non-unity build still works in CI.

## Verify

- Configure and build clean from an empty build directory
- No directory-scoped `include_directories`/`add_definitions`/`CMAKE_CXX_FLAGS` appends in new CMake
- Every dependency consumed through a namespaced imported target
- `ctest --output-on-failure` passes, and individual cases are selectable by name
- ASan/UBSan preset builds and passes
- `clang-format --dry-run --Werror` produces no diff

# Concurrency and verification

## Tooling

| Tool | Purpose |
|------|---------|
| `cmake` | Build system; `CMAKE_EXPORT_COMPILE_COMMANDS=ON` feeds every other tool |
| `clang-format` | Formatter, driven by the repo's `.clang-format` |
| `clang-tidy` | Lint (`bugprone-*`, `performance-*`, `modernize-*`, `cppcoreguidelines-*`) |
| ASan + UBSan | `-fsanitize=address,undefined`; TSan separately for threaded code |
| `gtest` / `catch2` | Unit tests |
| `include-what-you-use` | Cuts transitive-include creep that slows builds and hides dependencies |
| `ccache` | Compile cache; the single biggest iteration-speed win on a C++ tree |

Build with `-Wall -Wextra -Wpedantic -Wshadow -Wconversion` and treat warnings as errors in CI.


## Concurrency

- Lock through an RAII guard, never a bare `lock()`/`unlock()` pair -- an early return or a throw between them leaves the mutex held. `std::lock_guard` for a plain scope, `std::unique_lock` when the lock must be deferred, moved, or handed to a condition variable, `std::shared_lock` for reader access.
- **Name the guard.** `std::lock_guard<std::mutex>{m};` is a temporary that locks and unlocks before the next statement runs, leaving everything after it unguarded, and it compiles silently under `-Wall -Wextra -Wshadow`. `std::lock_guard<std::mutex>(m);` is worse-looking but harmless -- it parses as a declaration of a variable named `m` and fails to compile. Only `std::lock_guard<std::mutex> guard{m};` locks for the scope.
- Take multiple mutexes with one `std::scoped_lock(a, b)` (C++17), which applies a deadlock-avoidance algorithm. Two sequential guards impose a lock order that a second call site can invert.
- Always pass a predicate: `cv.wait(lock, [&]{ return ready; })`. A bare `wait` returns on spurious wakeup and on a notify that raced ahead of the waiter.
- Never call unknown code -- a user callback, a virtual, an observer notification -- while holding a lock. The callee may take another lock, re-enter, or block, and none of that is visible at the call site.
- A `mutex` member makes a class non-copyable and non-movable; decide whether the type is meant to be either before adding one.

Compile-time signal for these is near zero, so run anything threaded under TSan (see Testing) rather than trusting the warning bundle.


## Testing

- gtest is the default (`TEST(Suite, Case)`, `TEST_F` for fixtures). One test file per public surface.
- `EXPECT_*` to continue after failure, `ASSERT_*` where continuing would crash or cascade.
- `EXPECT_THROW`/`EXPECT_NO_THROW` for the exception contract; assert on the exception's type and message, not merely that something threw.
- Run the suite under ASan and UBSan in CI, and under TSan separately for anything threaded.

For generic test discipline (anti-patterns, real assertions, rationalization resistance), see the `ia-writing-tests` skill.


## Build and analysis

For CMake target design, sanitizer and warning presets, `clang-tidy` configuration (including which checks are fatal versus warn-only), and dependency handling, load [cmake-and-tooling.md](./cmake-and-tooling.md).

# C++ legibility: the deep rules and a worked refactor

Supplement to the SKILL.md body. Load when refactoring existing C++, when a review needs a rule stated precisely, or when deciding how far to push a decomposition.

Adapted from [write-legible-c](https://github.com/7etsuo/write-legible-c) by 7etsuo (MIT), with the C-specific error model replaced by C++ vocabulary types and the decomposition guard strengthened.

## Why these rules exist

Every rule targets one cost: the tokens and working memory needed to reason about any region of code. A rule earns its place by making a symbol greppable to few sites, keeping an edit local, or removing a path the reader must simulate.

The empirical case, briefly:

- Stripping identifiers degrades model performance even on execution tasks that should depend only on structure, so naming is a semantic channel and not decoration (arXiv 2510.03178). Human comprehension studies attribute up to a 30 percent effect to good names (Feitelson et al., ICPC).
- Models generate better against precise concise identifiers than verbose composites (arXiv 2508.06414).
- Model-perceived complexity, driven by semantic hierarchy depth and branching breadth, correlates with task performance after controlling for code length (arXiv 2602.07882).
- Standard complexity metrics predict whether generated code passes, which is why the budget below is numeric rather than a feeling (arXiv 2505.23953).

## The decomposition guard

State this before any splitting rule, and apply it as a filter on every proposed helper:

> If the most honest name for a candidate helper merely paraphrases its body, inline it and stop.

A helper earns existence by naming a concept, owning an error value, or isolating a side effect. Nothing else counts. Applied late, the line-count and nesting rules produce a hundred two-line functions and turn every read into a pointer chase.

C++ adds a second failure mode the C original does not have: a helper extracted as a *private member function* also widens the class. Prefer a file-local function in an anonymous namespace, or a `static` member, unless the helper genuinely needs the object's state. Every private member is another thing a reader of the header must scan past.

## Function rules

- One job per function. A contract comment needing the word "and" means two functions.
- Target 15 lines, hard cap 40. Nesting depth 2, guard clauses first.
- Past 4 parameters, the list is a struct trying to exist. In C++ that struct is often a named options type, which also kills the call-site ambiguity of three consecutive `bool` arguments.
- Classify every function as orchestrator (calls helpers, checks results, branches on named predicates), leaf (straight-line logic calling only accessors and pure utilities), or adapter (wraps exactly one foreign call and translates its convention). Never a mix.
- Cognitive complexity target 8, hard cap 15, by the Sonar rules that charge each break in linear flow and charge nesting progressively (G. Ann Campbell, SonarSource). The metric is a tripwire, not a goal.

## Naming

- Functions are verb_object. Predicates start `is_`/`has_` and are never negated.
- Precise beats verbose: `retry_count`, not `number_of_connection_retry_attempts`.
- Name length scales with the distance between declaration and last use.
- Units live in the name: `timeout_ms`, `max_payload_bytes`.
- Follow the repo's case convention exactly. Consistency across the file beats any personal preference between `snake_case` and `camelCase`.

## Comments

- Above every public declaration: what it does, ownership and nullability of every pointer or reference parameter, the failure modes, and any thread-safety guarantee. Never restate the signature.
- Inside bodies: comment why, never what.
- No commented-out code. Version control remembers.
- Name the subject rather than "this" or "the above" when more than one antecedent is in scope.

## Full pre-delivery checklist

Two standing exemptions, because a metric must never drive an interface change. The numeric items are tripwires for new or substantially rewritten *internal* code, not a licence to restructure untouched legacy a scoped fix passes through. And none of them justifies altering a released signature or anything ABI-stable — where a rule and a frozen boundary collide, the boundary wins and the deviation gets a comment at the declaration.

1. Any literal that is not 0 or 1? Name it (`constexpr`, not `#define`).
2. Any function over 40 lines, or nested past depth 2? Split it.
3. Any contract comment containing "and"? Split the function.
4. Every failure path reported through the module's one error model?
5. Each new error type: how many throw or return sites? Reduce toward one.
6. Any logic pasted twice? Extract it.
7. Any parameter list past 4, or two adjacent same-typed parameters a caller could swap? Struct it.
8. Header exposes only what callers need? Any implementation detail promotable to the `.cpp`?
9. Any function mixing helper calls with inline logic? Push the logic into a leaf.
10. Any helper whose name paraphrases its body? Inline it.
11. Any class that gained a destructor? Review all five special members.
12. Any `string_view` or reference member outliving its backing store?
13. Any raw `new`/`delete`, or a `shared_ptr` where `unique_ptr` suffices?
14. Any `using namespace` at namespace scope in a header?
15. Any loop that a named `<algorithm>` call states more clearly?
16. Any recursion reachable from external input without an explicit depth bound?

## Worked refactor: good but not good enough

The function below passes generic review: short, flat, guarded, no magic numbers. It still fails.

```cpp
uint16_t Map::eat(Position pos)
{
    Cell cell;
    if (!isInside(pos))
        return 0;
    cell = cells_[pos.row][pos.col];
    if (cell == Cell::Pellet) {
        cells_[pos.row][pos.col] = Cell::Empty;
        pelletCount_--;
        return kScorePellet;
    }
    if (cell == Cell::Power) {
        cells_[pos.row][pos.col] = Cell::Empty;
        pelletCount_--;
        return kScorePower;
    }
    return 0;
}
```

The tells, in order of weight:

1. **The consume block is pasted twice.** Clearing the cell and decrementing the count is one concept in two places. An editor later adding a side effect to consumption patches one copy and misses the other, because nothing links them.
2. **The branches encode data as control flow.** Cell type to score is a mapping, not logic. The next cell type should cost one line, not one pasted block.
3. **Three concepts interleave**: deciding edibility, awarding score, and mutating the grid. No single question about this function has a single home.
4. **`Cell cell;` is left uninitialized above the guard.** Default-initializing a scoped enum with automatic storage leaves an indeterminate value, so the declaration carries no information at all until the assignment below it. Declare at the point where the first valid value exists.

### Stage one: decompose, signature preserved

```cpp
namespace {

bool isEdible(Cell cell)
{
    return cell == Cell::Pellet || cell == Cell::Power;
}

uint16_t scoreFor(Cell cell)
{
    switch (cell) {
    case Cell::Pellet: return kScorePellet;
    case Cell::Power:  return kScorePower;
    default:           return 0;
    }
}

} // namespace

uint16_t Map::eat(Position pos)
{
    if (!isInside(pos))
        return 0;

    const Cell cell = cellAt(pos);
    if (!isEdible(cell))
        return 0;

    consumeCell(pos);
    return scoreFor(cell);
}

void Map::consumeCell(Position pos)
{
    setCell(pos, Cell::Empty);
    pelletCount_--;
}
```

The anonymous-namespace block sits **above** its call site deliberately. Unlike a member function, a free function must be declared before it is used, and ADL does not rescue it in non-template code: defining these below `Map::eat` is a compile error, not a style preference.

`isEdible` and `scoreFor` are pure functions of a `Cell` and need nothing from the object, so they live in an anonymous namespace in the `.cpp` rather than widening the class.

**The proof is change cost, not line count.** Add a fruit cell: the original grows a third pasted block and the next editor patches two of three copies. The refactor grows one line in `isEdible` and one in `scoreFor`. If edibility is exactly "scores nonzero", both collapse into one `constexpr` lookup table with zero branches; state that invariant in a comment above the table.

### Stage two: separate failure from result

Stage one kept one violation deliberately: the return value fuses failure with score, returning 0 for an out-of-bounds position and for an ordinary empty cell alike. A caller cannot distinguish a bug from a normal move.

```cpp
/* Consumes the cell at pos if edible and returns the score awarded,
 * zero when nothing edible is there. Returns nullopt when pos is
 * outside the grid. */
std::optional<uint16_t> Map::eat(Position pos)
{
    if (!isInside(pos))
        return std::nullopt;

    const Cell cell = cellAt(pos);
    if (!isEdible(cell))
        return 0;

    consumeCell(pos);
    return scoreFor(cell);
}
```

This is where C++ diverges from the C original, which needs a status enum and an out-parameter to say the same thing. `std::optional` carries "no answer" in the type, so the caller cannot read a score that was never produced. Where the failure has more than one cause worth distinguishing, use `std::expected<uint16_t, MapError>` (C++23) or the project's equivalent instead, and give each error one construction site.

`cellAt` and `setCell` earn their existence by owning every touch of the grid storage, so the row-major indexing convention lives in two adjacent lines and "what mutates cells" greps to one answer. That is the sanctioned accessor exception from the decomposition guard, and the only shape where a one-line member survives the name test.

### When to stop

| Situation | Stop at |
|---|---|
| Bug fix inside an existing function | The smallest behavior-preserving decomposition that removes the duplication causing the bug |
| Feature work touching the function | Stage one |
| The public API is being designed or is not yet released | Stage two |
| The signature is frozen by ABI or a released header | Stage one, with a deviation comment above the declaration |

A standard is grown by feeding it code that breaks it, and each break becomes a rule or an amendment. Apply the same process to any codebase adopting this document.

# Value and interface design

## Const correctness and value semantics

- `const` by default on locals, member functions, and reference parameters.
- Pass by value for types that are cheap to move (`std::string`, `std::vector`) when the function stores the argument; pass by `const&` when it only reads. Do not pass by `const&` and then copy inside.
- `std::string_view` for read-only string parameters, with one rule attached: never store one unless the backing buffer is guaranteed to outlive the view. A `string_view` member is a dangling reference waiting for a temporary.
- Mark member functions `const` and `noexcept` where true. `noexcept` on move operations is what lets `std::vector` move rather than copy on reallocation.


## Error handling

Pick one model per module and hold it at the boundary.

- **Exceptions** where the project allows them: throw types deriving from `std::exception`, throw by value, catch by `const&`. Use them for genuinely exceptional conditions, not for control flow.
- **`std::optional<T>`** for "absent is normal". **`std::expected<T, E>`** (C++23) or a project equivalent for "failed with a reason".
- **Error codes** in exception-free builds, with `[[nodiscard]]` on every returning function so an ignored failure is a warning.
- Mark anything that must not throw `noexcept`, and mean it: an escaping exception calls `std::terminate`.
- A constructor that can fail either throws or does not exist. Two-phase `init()` construction produces objects with an invalid state that every method must then check. Prefer a static factory returning `optional`/`expected`.


## API design

The decisions that break callers, learned the expensive way:

- **Three decisions break callers when they go wrong**: `explicit` on a single-argument constructor (decide at introduction, since adding it later is source-breaking), removing an overload (keep the narrow one and delegate), and an overload that silently ignores part of its argument (delete it or `static_assert` instead). Rationale and the full evolution rules are in the reference below.
- Prefer free functions over members where they do not need private access; they extend without touching the class.
- Return by value and let the compiler elide. Out-parameters exist for multiple returns and for reuse of a caller's buffer, not as an optimization.

For `extern "C"` boundaries, exception containment, PIMPL, and ABI-stable headers, load [api-and-abi.md](./api-and-abi.md).


## Templates and generic code

Use a template when at least three concrete instantiations exist or are certain. Before that, a concrete type is clearer and compiles faster.

- Constrain with C++20 concepts where available, `static_assert` plus type traits otherwise. An unconstrained template fails deep inside instantiation with an error nobody can read.
- `if constexpr` over tag dispatch and SFINAE where the standard allows it.
- Keep template definitions out of widely-included headers when the instantiation cost is real; explicit instantiation in one translation unit is often the right trade.
- Perfect forwarding (`T&&` plus `std::forward`) only in genuinely forwarding code. A forwarding reference in a constructor hijacks the copy constructor and produces baffling overload resolution.


## Standard library

- Prefer `<algorithm>` and ranges over hand-written loops; a named algorithm states intent that an index loop hides.
- `std::vector` unless measurement says otherwise. `reserve()` when the final size is known.
- Structured bindings for pair and tuple returns; a named struct for anything a caller will read twice.
- Use `std::move` only where the source is genuinely dead afterwards. Never depend on an *unspecified* post-move value; destroy, reassign, or invoke only operations whose post-move contract is documented. Some types do specify one (`unique_ptr` is null, `future` is invalid), and relying on those is fine.
- Never return `std::move(local)`: it defeats copy elision.
- Never call unknown code -- a user callback, a virtual, a comparator, a visitor -- from inside a loop over a container the callee can reach. Insertion invalidates iterators and pointers into the storage, and a callback that mutates the container being walked leaves the loop reading freed memory whose bytes usually still look plausible, so the first symptom is wrong output rather than a crash. Iterate a copy, or index by position and re-check `size()` after every reentrant call, and where elements are owned indirectly, take a strong reference on each one before the pass that can drop the last owner.
- A checked downcast per element is dispatch, not work. `dynamic_pointer_cast` from `shared_from_this()` costs an RTTI walk plus an atomic refcount round trip on a temporary, and in one rows-by-columns decode loop it measured about 18% of retired instructions. Where a type tag has already proved the concrete class, `static_cast` the raw pointer -- but keep the checked cast for any type whose tag-to-class mapping has moved across a library version, where the null return is the crash guard.
- An empty `std::string_view` may have `data() == nullptr`, which the standard permits, and `memcpy` declares its source non-null regardless of the size argument. Every libc no-ops a zero-length copy in practice, so the defect surfaces only as a UBSan diagnostic on each empty append. Guard the copy with a size check rather than suppressing the check.


## Legibility

Function decomposition, naming as a greppability contract, the name test that stops over-decomposition, contract comments, and a worked refactor with its change-cost proof: load [legibility-standard.md](./legibility-standard.md).