diff --git a/.agents/skills/cpp-code-review/SKILL.md b/.agents/skills/cpp-code-review/SKILL.md new file mode 100644 index 0000000..4308bce --- /dev/null +++ b/.agents/skills/cpp-code-review/SKILL.md @@ -0,0 +1,59 @@ +--- +name: cpp-code-review +description: >- + Review C++ (and C interop) code and PRs for undefined behavior, memory/ + lifetime bugs, concurrency, error handling and modern idioms. Use whenever + reviewing or auditing C++ code. +--- + +# C++ Code Review + +Review C++ changes for real bugs first, style last. Be concise and cite +file:line. + +## Workflow + +1. **Scope**: identify what changed (diff, PR, or files). Check the build files + (`CMakeLists.txt`, etc.) for the C++ standard (`-std=`/ `CMAKE_CXX_STANDARD`) + and compiler. Don't suggest features newer than the project's standard. +2. **Tooling**: if you can run commands, run what's available and fold results + in (don't re-report what they catch): + - Build with `-Wall -Wextra -Wpedantic` (or `/W4` on MSVC) + - `clang-tidy` (with `bugprone-*`, `cppcoreguidelines-*`, `modernize-*`) + - Tests under sanitizers: `-fsanitize=address,undefined`; separately + `-fsanitize=thread` for threaded code +3. **Review by priority**: read only the reference files that apply: + +| Change touches | Read | +| ------------------------------------------------------------------------------- | ---------------------------------- | +| Raw pointers, `new`/`delete`, buffers, casts, C APIs, references to temporaries | `references/memory-ub.md` | +| Threads, atomics, mutexes, `std::async`, coroutines | `references/concurrency.md` | +| Exceptions, error codes, classes, templates, API design, integer types | `references/correctness-idioms.md` | +| Naming, signatures, includes, class layout, conventions | `references/project-style.md` | +| Unsure whether a finding is real | `references/false-positives.md` | +| Need to explain or verify a language/library rule | `references/cppreference.md` | + +4. **Report**: use `references/report-template.md`. + +## Priorities (in order) + +1. Undefined behavior and memory safety (overflow, use-after-free, dangling, + out-of-bounds) +2. Security (untrusted input reaching buffers, format strings, integer + conversions) +3. Concurrency bugs (data races, deadlocks) +4. Resource leaks and exception safety +5. API/correctness and performance (copies, allocations) +6. Modern idioms and readability + +## Rules + +- Only flag issues you can explain concretely. No speculation. +- Don't comment on formatting (whitespace, line breaks); that is + `clang-format`'s job. Do enforce the project's coding conventions; see + `references/project-style.md`. +- Suggest a fix with a short code snippet when possible. +- Before asserting a language or library rule (e.g. iterator invalidation, UB, + which header declares a type), verify it on cppreference. Link the page in the + finding instead of writing a long explanation. See + `references/cppreference.md` for how to look things up. diff --git a/.agents/skills/cpp-code-review/references/concurrency.md b/.agents/skills/cpp-code-review/references/concurrency.md new file mode 100644 index 0000000..6b4429d --- /dev/null +++ b/.agents/skills/cpp-code-review/references/concurrency.md @@ -0,0 +1,25 @@ +# Concurrency + +Docs: cppreference "Concurrency support library", `std::memory_order`, +`std::atomic`. + +- Shared mutable data is protected by a mutex or is `std::atomic`; otherwise + it's a data race (UB). +- Use `std::scoped_lock` / `std::lock_guard` / `std::unique_lock`, never manual + `lock()`/`unlock()`. +- Multiple mutexes locked together with `std::scoped_lock(a, b)` or in a + consistent order. +- No callbacks, I/O, or user code invoked while holding a lock. +- Condition variables wait with a predicate (spurious wakeups). +- `std::thread` is joined or detached before destruction; prefer `std::jthread` + (C++20). +- `std::async` futures: the returned future's destructor blocks; don't discard + it by accident. +- Atomics: `memory_order_relaxed` only when no other memory depends on the + value. +- Lambdas passed to threads don't capture locals by reference that may die + first. +- Static local init is thread-safe; other lazy init (double-checked locking) + needs atomics or `std::call_once`. +- Coroutines: references/pointer parameters may dangle after the first + suspension. diff --git a/.agents/skills/cpp-code-review/references/correctness-idioms.md b/.agents/skills/cpp-code-review/references/correctness-idioms.md new file mode 100644 index 0000000..3575512 --- /dev/null +++ b/.agents/skills/cpp-code-review/references/correctness-idioms.md @@ -0,0 +1,41 @@ +# Correctness & Modern Idioms + +Docs: cppreference language/library pages; C++ Core Guidelines. + +## Errors & exceptions + +- Errors aren't silently ignored (unchecked return codes, empty `catch (...)`). +- Mark functions whose result must be used `[[nodiscard]]`. +- Exception safety: on throw, no leaks and objects stay valid (RAII gives this + for free). +- Destructors and move operations are `noexcept`; no throwing from destructors. +- Catch exceptions by const reference (`catch (std::exception const& e)`). +- Consider `std::optional` / `std::expected` (C++23) for expected failures, per + project style. + +## Types & integers + +- Fixed-width integers: use the project's aliases (`u32`, `i64`, ...) instead of + `std::uint32_t` etc., e.g. for sizes in protocols, files, and hardware. +- Signed/unsigned comparison and mixing (`int` vs `size_t`); use `std::cmp_less` + etc. (C++20) or `std::ssize`. +- Narrowing conversions are explicit and checked; prefer `{}` initialization to + catch them. +- No implicit conversions via single-arg constructors; mark them `explicit`. +- `enum class` over plain `enum`. + +## API & classes + +- `const` correctness: methods, references, and parameters. +- Pass cheap types by value, large read-only by `const&`, sinks by value + + `std::move`. +- `override` on virtual overrides; `final` where intended. +- No object slicing (passing derived by value as base). +- Header hygiene: include what you use, no `using namespace` in headers. + +## Modern idioms + +- Prefer standard algorithms and range-for over hand-rolled loops. +- `constexpr` for compile-time constants instead of macros. +- `std::string_view` / `std::span` for non-owning parameters (watch lifetimes). +- New logic has tests; edge cases (empty, max, error path) are covered. diff --git a/.agents/skills/cpp-code-review/references/cppreference.md b/.agents/skills/cpp-code-review/references/cppreference.md new file mode 100644 index 0000000..00009ab --- /dev/null +++ b/.agents/skills/cpp-code-review/references/cppreference.md @@ -0,0 +1,40 @@ +# cppreference Lookup + +Use cppreference to verify rules and to link in findings. Fetch pages with web +fetch when unsure. + +## How to look things up + +- Search any symbol, header, or keyword: + `https://cppreference.com/index.php?title=Special%3ASearch&search=` e.g. + `...&search=cstdint`, `...&search=std%3A%3Aspan`, `...&search=memory_order` + The results page links to C++ and C entries separately; pick the right column. +- Each page lists which standard (C++11/17/20/23) added a feature; check it + against the project's `-std`. + +## Entry points + +- Language: https://en.cppreference.com/cpp/language +- Standard library headers: https://en.cppreference.com/cpp/header +- Fixed-width integers ``: + https://en.cppreference.com/cpp/header/cstdint +- Undefined behavior: https://en.cppreference.com/cpp/language/ub +- Object lifetime: https://en.cppreference.com/cpp/language/lifetime +- Initialization: https://en.cppreference.com/cpp/language/initialization +- Evaluation order: https://en.cppreference.com/cpp/language/eval_order +- Exceptions: https://en.cppreference.com/cpp/language/exceptions +- Memory / smart pointers: https://en.cppreference.com/cpp/memory +- Containers (iterator invalidation tables): + https://en.cppreference.com/cpp/container +- Concurrency: https://en.cppreference.com/cpp/thread +- Memory order: https://en.cppreference.com/cpp/atomic/memory_order +- Compiler support by standard: https://en.cppreference.com/cpp/compiler_support +- C reference (for C interop): https://en.cppreference.com/c + +## Other references + +- C++ Core Guidelines: + https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines +- clang-tidy checks: https://clang.llvm.org/extra/clang-tidy/checks/list.html + +If a deep link 404s, use the search URL above instead. diff --git a/.agents/skills/cpp-code-review/references/false-positives.md b/.agents/skills/cpp-code-review/references/false-positives.md new file mode 100644 index 0000000..bef4625 --- /dev/null +++ b/.agents/skills/cpp-code-review/references/false-positives.md @@ -0,0 +1,20 @@ +# Avoiding False Positives + +Before reporting, check: + +- A raw pointer that is non-owning (an observer) is fine; only owning raw + pointers are a problem. +- Unsigned integer overflow is defined (wraps); only signed overflow is UB. +- `std::move` on a `const` object or a return of a local is a + pessimization/no-op, not a bug. +- Copy elision: returning a local by value is fine; don't suggest `std::move` on + return. +- Static locals are initialized thread-safely since C++11. +- Code guarded by `#ifdef` or feature macros may target a different standard or + platform. +- If the compiler (with warnings enabled) would reject or warn on it, defer to + the compiler. +- If you're unsure a construct is UB, look it up on cppreference before + reporting. + +If still unsure, mark the finding as **Question** instead of **Issue**. diff --git a/.agents/skills/cpp-code-review/references/memory-ub.md b/.agents/skills/cpp-code-review/references/memory-ub.md new file mode 100644 index 0000000..f82658e --- /dev/null +++ b/.agents/skills/cpp-code-review/references/memory-ub.md @@ -0,0 +1,45 @@ +# Memory Safety & Undefined Behavior + +Docs: cppreference "Undefined behavior", "Object lifetime", "Smart pointers" +(see `cppreference.md`). + +## Ownership & lifetime + +- No owning raw pointers; use `std::unique_ptr` / `std::shared_ptr`, containers, + or RAII wrappers. +- No `new`/`delete` in application code; use `std::make_unique` / + `std::make_shared`. +- No references, pointers, `string_view`, or `span` that outlive what they point + to (returning views of locals or temporaries, lambdas capturing locals by `&` + that escape). +- Iterators/pointers/references into containers aren't used after an operation + that may invalidate them (`push_back`, `insert`, `erase`, `rehash`). +- No use after `std::move` except to reassign or destroy. +- Rule of 0/3/5: if a class defines a destructor, copy, or move, it handles all + of them correctly (or `= delete`s them). +- Base classes used polymorphically have a `virtual` (or protected) destructor. + +## Bounds & buffers + +- Indexing is bounds-checked or provably in range; prefer `.at()` at trust + boundaries, `std::span`, range-for. +- No `strcpy`, `sprintf`, `gets`, unchecked `memcpy`; sizes come from the + destination. +- Off-by-one in loop bounds and null terminators. + +## Undefined behavior + +- Signed integer overflow; shifts by negative or >= bit width. +- Uninitialized variables (especially scalars and POD members). +- Strict aliasing violations; use `std::memcpy` or `std::bit_cast` (C++20) for + type punning. +- `reinterpret_cast` / C-style casts that change object type; misaligned access. +- Null dereference, including `.get()` on empty smart pointers. +- Unsequenced modifications (`i = i++`-style). + +## C interop + +- `extern "C"` boundaries: no exceptions propagate into C code. +- Memory allocated by C (`malloc`) freed with `free`, not `delete` (and vice + versa). +- C strings: buffer size and termination checked. diff --git a/.agents/skills/cpp-code-review/references/project-style.md b/.agents/skills/cpp-code-review/references/project-style.md new file mode 100644 index 0000000..7069974 --- /dev/null +++ b/.agents/skills/cpp-code-review/references/project-style.md @@ -0,0 +1,31 @@ +# Project Conventions + +## Coding Conventions + +- **Language:** C++23 +- **Primitive types:** use fixed-width type aliases (`u32`, `i64`, ...) instead + of `std::uint32_t` etc. +- **Trailing return type** for function signatures (e.g. `auto fn() -> void`) +- **4-space indentation** +- **No semicolons after closing braces** for namespaces/classes +- `auto` for obvious types (e.g. `auto main(...) -> int`) +- **East const** (e.g. `char const*` not `const char*`) +- **Trailing return type** for all function definitions, including operators + (e.g. `auto operator=(T&&) noexcept -> T&`) +- **Public members first** in class declarations, private members at the bottom +- **Private data members** start with `m_` (e.g. `m_state`) +- `<>` includes only for system headers (std, OS, etc.) +- `""` includes for third-party dependencies (e.g. `fmt`, `nlohmann/json`) +- **Naming:** `snake_case` for variables, functions, and classes +- **Naming:** `SCREAMING_SNAKE_CASE` only for macros and constants +- **Header extensions:** C++ headers use `.hpp`, C headers use `.h` +- Include order: + 1. C++ standard library headers (``, ``, etc.) + 2. _(blank line)_ + 3. C standard library headers (``, ``, etc.) + 4. _(blank line)_ + 5. OS-specific headers (Windows API, POSIX, etc.) + 6. _(blank line)_ + 7. Third-party dependencies (`"fmt/core.h"`, etc.) + 8. _(blank line)_ + 9. Local/project headers diff --git a/.agents/skills/cpp-code-review/references/report-template.md b/.agents/skills/cpp-code-review/references/report-template.md new file mode 100644 index 0000000..9793667 --- /dev/null +++ b/.agents/skills/cpp-code-review/references/report-template.md @@ -0,0 +1,23 @@ +# Report Template + +``` +## Summary +One or two sentences: overall verdict and biggest risk. + +## Findings +### [Critical|High|Medium|Low] Short title: path/file.cpp:LINE +What's wrong and why it matters (1-3 sentences). +Fix: + // suggested code +Ref: + +## Questions +- Things that need the author's intent to judge. + +## Nits (optional) +- Minor idiom/readability notes. +``` + +Severity guide: **Critical** = UB, memory corruption, security; **High** = data +race, deadlock, leak, crash; **Medium** = error handling, API, perf; **Low** = +idioms. diff --git a/AGENTS.md b/AGENTS.md index d244adb..7f1648f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,36 +27,6 @@ cmake --build build --target Ex11 Tests (optional): configure with `-DBUILD_TESTING=ON`, then run `ninja check` in the build directory. -## Coding Conventions - -- **Language:** C++23 -- **Primitive types:** use fixed-width type aliases (`u32`, `i64`, ...) instead - of `std::uint32_t` etc. -- **Trailing return type** for function signatures (e.g. `auto fn() -> void`) -- **4-space indentation** -- **No semicolons after closing braces** for namespaces/classes -- `auto` for obvious types (e.g. `auto main(...) -> int`) -- **East const** (e.g. `char const*` not `const char*`) -- **Trailing return type** for all function definitions, including operators - (e.g. `auto operator=(T&&) noexcept -> T&`) -- **Public members first** in class declarations, private members at the bottom -- **Private data members** start with `m_` (e.g. `m_state`) -- `<>` includes only for system headers (std, OS, etc.) -- `""` includes for third-party dependencies (e.g. `fmt`, `nlohmann/json`) -- **Naming:** `snake_case` for variables, functions, and classes -- **Naming:** `SCREAMING_SNAKE_CASE` only for macros and constants -- **Header extensions:** C++ headers use `.hpp`, C headers use `.h` -- Include order: - 1. C++ standard library headers (``, ``, etc.) - 2. _(blank line)_ - 3. C standard library headers (``, ``, etc.) - 4. _(blank line)_ - 5. OS-specific headers (Windows API, POSIX, etc.) - 6. _(blank line)_ - 7. Third-party dependencies (`"fmt/core.h"`, etc.) - 8. _(blank line)_ - 9. Local/project headers - ## Shell Scripts - Always use `#!/bin/sh` shebang for shell scripts