docs(agents): add cpp-code-review skill
Add a code review skill with reference files for memory safety, concurrency, correctness idioms, false positives, cppreference lookup, and report format. Move the coding conventions from AGENTS.md into the skill's project-style reference so reviews enforce them. Co-authored-by: Qwen 3.8 27B <qwen3.8_27b-q3_k_xl@lmstudio.local>
This commit is contained in:
@@ -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.
|
||||||
@@ -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.
|
||||||
@@ -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.
|
||||||
@@ -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=<TERM>` 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 `<cstdint>`:
|
||||||
|
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.
|
||||||
@@ -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**.
|
||||||
@@ -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.
|
||||||
@@ -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 (`<chrono>`, `<vector>`, etc.)
|
||||||
|
2. _(blank line)_
|
||||||
|
3. C standard library headers (`<stdlib.h>`, `<string.h>`, 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
|
||||||
@@ -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: <cppreference link if helpful>
|
||||||
|
|
||||||
|
## 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.
|
||||||
@@ -27,36 +27,6 @@ cmake --build build --target Ex11
|
|||||||
Tests (optional): configure with `-DBUILD_TESTING=ON`, then run `ninja check` in
|
Tests (optional): configure with `-DBUILD_TESTING=ON`, then run `ninja check` in
|
||||||
the build directory.
|
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 (`<chrono>`, `<vector>`, etc.)
|
|
||||||
2. _(blank line)_
|
|
||||||
3. C standard library headers (`<stdlib.h>`, `<string.h>`, 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
|
## Shell Scripts
|
||||||
|
|
||||||
- Always use `#!/bin/sh` shebang for shell scripts
|
- Always use `#!/bin/sh` shebang for shell scripts
|
||||||
|
|||||||
Reference in New Issue
Block a user