feat: add C and C++ code review guides

2026-03-22 02:19:32 +08:00 · 2026-01-11 17:54:54 +08:00
parent 25bb415cef
commit c687f500b7
2 changed files with 670 additions and 0 deletions
--- a/reference/c.md
+++ b/reference/c.md
@@ -0,0 +1,285 @@
+# C Code Review Guide
+
+> C code review guide focused on memory safety, undefined behavior, and portability. Examples assume C11.
+
+## Table of Contents
+
+- [Pointer and Buffer Safety](#pointer-and-buffer-safety)
+- [Ownership and Resource Management](#ownership-and-resource-management)
+- [Undefined Behavior Pitfalls](#undefined-behavior-pitfalls)
+- [Integer Types and Overflow](#integer-types-and-overflow)
+- [Error Handling](#error-handling)
+- [Concurrency](#concurrency)
+- [Macros and Preprocessor](#macros-and-preprocessor)
+- [API Design and Const](#api-design-and-const)
+- [Tooling and Build Checks](#tooling-and-build-checks)
+- [Review Checklist](#review-checklist)
+
+---
+
+## Pointer and Buffer Safety
+
+### Always carry size with buffers
+
+```c
+// ? Bad: ignores destination size
+bool copy_name(char *dst, size_t dst_size, const char *src) {
+    strcpy(dst, src);
+    return true;
+}
+
+// ? Good: validate size and terminate
+bool copy_name(char *dst, size_t dst_size, const char *src) {
+    size_t len = strlen(src);
+    if (len + 1 > dst_size) {
+        return false;
+    }
+    memcpy(dst, src, len + 1);
+    return true;
+}
+```
+
+### Avoid dangerous APIs
+
+Prefer `snprintf`, `fgets`, and explicit bounds over `gets`, `strcpy`, or `sprintf`.
+
+```c
+// ? Bad: unbounded write
+sprintf(buf, "%s", input);
+
+// ? Good: bounded write
+snprintf(buf, buf_size, "%s", input);
+```
+
+### Use the right copy primitive
+
+```c
+// ? Bad: memcpy with overlapping regions
+memcpy(dst, src, len);
+
+// ? Good: memmove handles overlap
+memmove(dst, src, len);
+```
+
+---
+
+## Ownership and Resource Management
+
+### One allocation, one free
+
+Track ownership and clean up on every error path.
+
+```c
+// ? Good: cleanup label avoids leaks
+int load_file(const char *path) {
+    int rc = -1;
+    FILE *f = NULL;
+    char *buf = NULL;
+
+    f = fopen(path, "rb");
+    if (!f) {
+        goto cleanup;
+    }
+    buf = malloc(4096);
+    if (!buf) {
+        goto cleanup;
+    }
+
+    if (fread(buf, 1, 4096, f) == 0) {
+        goto cleanup;
+    }
+
+    rc = 0;
+
+cleanup:
+    free(buf);
+    if (f) {
+        fclose(f);
+    }
+    return rc;
+}
+```
+
+---
+
+## Undefined Behavior Pitfalls
+
+### Common UB patterns
+
+```c
+// ? Bad: use after free
+char *p = malloc(10);
+free(p);
+p[0] = 'a';
+
+// ? Bad: uninitialized read
+int x;
+if (x > 0) { /* UB */ }
+
+// ? Bad: signed overflow
+int sum = a + b;
+```
+
+### Avoid pointer arithmetic past the object
+
+```c
+// ? Bad: pointer past the end then dereference
+int arr[4];
+int *p = arr + 4;
+int v = *p; // UB
+```
+
+---
+
+## Integer Types and Overflow
+
+### Avoid signed/unsigned surprises
+
+```c
+// ? Bad: negative converted to large size_t
+int len = -1;
+size_t n = len;
+
+// ? Good: validate before converting
+if (len < 0) {
+    return -1;
+}
+size_t n = (size_t)len;
+```
+
+### Check for overflow in size calculations
+
+```c
+// ? Bad: potential overflow in multiplication
+size_t bytes = count * sizeof(Item);
+
+// ? Good: check before multiplying
+if (count > SIZE_MAX / sizeof(Item)) {
+    return NULL;
+}
+size_t bytes = count * sizeof(Item);
+```
+
+---
+
+## Error Handling
+
+### Always check return values
+
+```c
+// ? Bad: ignore errors
+fread(buf, 1, size, f);
+
+// ? Good: handle errors
+size_t read = fread(buf, 1, size, f);
+if (read != size && ferror(f)) {
+    return -1;
+}
+```
+
+### Consistent error contracts
+
+- Use a clear convention: 0 for success, negative for failure.
+- Document ownership rules on success and failure.
+- If using `errno`, set it only for actual failures.
+
+---
+
+## Concurrency
+
+### volatile is not synchronization
+
+```c
+// ? Bad: data race
+volatile int stop = 0;
+void worker(void) {
+    while (!stop) { /* ... */ }
+}
+
+// ? Good: C11 atomics
+_Atomic int stop = 0;
+void worker(void) {
+    while (!atomic_load(&stop)) { /* ... */ }
+}
+```
+
+### Use mutexes for shared state
+
+Protect shared data with `pthread_mutex_t` or equivalent. Avoid holding locks while doing I/O.
+
+---
+
+## Macros and Preprocessor
+
+### Parenthesize arguments
+
+```c
+// ? Bad: macro with side effects
+#define MIN(a, b) ((a) < (b) ? (a) : (b))
+int x = MIN(i++, j++);
+
+// ? Good: static inline function
+static inline int min_int(int a, int b) {
+    return a < b ? a : b;
+}
+```
+
+---
+
+## API Design and Const
+
+### Const-correctness and sizes
+
+```c
+// ? Good: explicit size and const input
+int hash_bytes(const uint8_t *data, size_t len, uint8_t *out);
+```
+
+### Document nullability
+
+Clearly document whether pointers may be NULL. Prefer returning error codes instead of NULL when possible.
+
+---
+
+## Tooling and Build Checks
+
+```bash
+# Warnings
+clang -Wall -Wextra -Werror -Wconversion -Wshadow -std=c11 ...
+
+# Sanitizers (debug builds)
+clang -fsanitize=address,undefined -fno-omit-frame-pointer -g ...
+clang -fsanitize=thread -fno-omit-frame-pointer -g ...
+
+# Static analysis
+clang-tidy src/*.c -- -std=c11
+cppcheck --enable=warning,performance,portability src/
+
+# Formatting
+clang-format -i src/*.c include/*.h
+```
+
+---
+
+## Review Checklist
+
+### Memory and UB
+- [ ] All buffers have explicit size parameters
+- [ ] No out-of-bounds access or pointer arithmetic past objects
+- [ ] No use after free or uninitialized reads
+- [ ] Signed overflow and shift rules are respected
+
+### API and Design
+- [ ] Ownership rules are documented and consistent
+- [ ] const-correctness is applied for inputs
+- [ ] Error contracts are clear and consistent
+
+### Concurrency
+- [ ] No data races on shared state
+- [ ] volatile is not used for synchronization
+- [ ] Locks are held for minimal time
+
+### Tooling and Tests
+- [ ] Builds clean with warnings enabled
+- [ ] Sanitizers run on critical code paths
+- [ ] Static analysis results are addressed
--- a/reference/cpp.md
+++ b/reference/cpp.md
@@ -0,0 +1,385 @@
+# C++ Code Review Guide
+
+> C++ code review guide focused on memory safety, lifetime, API design, and performance. Examples assume C++17/20.
+
+## Table of Contents
+
+- [Ownership and RAII](#ownership-and-raii)
+- [Lifetime and References](#lifetime-and-references)
+- [Copy and Move Semantics](#copy-and-move-semantics)
+- [Const-Correctness and API Design](#const-correctness-and-api-design)
+- [Error Handling and Exception Safety](#error-handling-and-exception-safety)
+- [Concurrency](#concurrency)
+- [Performance and Allocation](#performance-and-allocation)
+- [Templates and Type Safety](#templates-and-type-safety)
+- [Tooling and Build Checks](#tooling-and-build-checks)
+- [Review Checklist](#review-checklist)
+
+---
+
+## Ownership and RAII
+
+### Prefer RAII and smart pointers
+
+Use RAII to express ownership. Default to `std::unique_ptr`, use `std::shared_ptr` only for shared lifetime.
+
+```cpp
+// ? Bad: manual new/delete with early returns
+Foo* make_foo() {
+    Foo* foo = new Foo();
+    if (!foo->Init()) {
+        delete foo;
+        return nullptr;
+    }
+    return foo;
+}
+
+// ? Good: RAII with unique_ptr
+std::unique_ptr<Foo> make_foo() {
+    auto foo = std::make_unique<Foo>();
+    if (!foo->Init()) {
+        return {};
+    }
+    return foo;
+}
+```
+
+### Wrap C resources
+
+```cpp
+// ? Good: wrap FILE* with unique_ptr
+using FilePtr = std::unique_ptr<FILE, decltype(&fclose)>;
+
+FilePtr open_file(const char* path) {
+    return FilePtr(fopen(path, "rb"), &fclose);
+}
+```
+
+---
+
+## Lifetime and References
+
+### Avoid dangling references and views
+
+`std::string_view` and `std::span` do not own data. Make sure the owner outlives the view.
+
+```cpp
+// ? Bad: returning string_view to a temporary
+std::string_view bad_view() {
+    std::string s = make_name();
+    return s; // dangling
+}
+
+// ? Good: return owning string
+std::string good_name() {
+    return make_name();
+}
+
+// ? Good: view tied to caller-owned data
+std::string_view good_view(const std::string& s) {
+    return s;
+}
+```
+
+### Lambda captures
+
+```cpp
+// ? Bad: capture reference that escapes
+std::function<void()> make_task() {
+    int value = 42;
+    return [&]() { use(value); }; // dangling
+}
+
+// ? Good: capture by value
+std::function<void()> make_task() {
+    int value = 42;
+    return [value]() { use(value); };
+}
+```
+
+---
+
+## Copy and Move Semantics
+
+### Rule of 0/3/5
+
+Prefer the Rule of 0 by using RAII types. If you own a resource, define or delete copy and move operations.
+
+```cpp
+// ? Bad: raw ownership with default copy
+struct Buffer {
+    int* data;
+    size_t size;
+    explicit Buffer(size_t n) : data(new int[n]), size(n) {}
+    ~Buffer() { delete[] data; }
+    // copy ctor/assign are implicitly generated -> double delete
+};
+
+// ? Good: Rule of 0 with std::vector
+struct Buffer {
+    std::vector<int> data;
+    explicit Buffer(size_t n) : data(n) {}
+};
+```
+
+### Delete unwanted copies
+
+```cpp
+struct Socket {
+    Socket() = default;
+    ~Socket() { close(); }
+
+    Socket(const Socket&) = delete;
+    Socket& operator=(const Socket&) = delete;
+    Socket(Socket&&) noexcept = default;
+    Socket& operator=(Socket&&) noexcept = default;
+};
+```
+
+---
+
+## Const-Correctness and API Design
+
+### Use const and explicit
+
+```cpp
+class User {
+public:
+    const std::string& name() const { return name_; }
+    void set_name(std::string name) { name_ = std::move(name); }
+
+private:
+    std::string name_;
+};
+
+struct Millis {
+    explicit Millis(int v) : value(v) {}
+    int value;
+};
+```
+
+### Avoid object slicing
+
+```cpp
+struct Shape { virtual ~Shape() = default; };
+struct Circle : Shape { void draw() const; };
+
+// ? Bad: slices Circle into Shape
+void draw(Shape shape);
+
+// ? Good: pass by reference
+void draw(const Shape& shape);
+```
+
+### Use override and final
+
+```cpp
+struct Base {
+    virtual void run() = 0;
+};
+
+struct Worker final : Base {
+    void run() override {}
+};
+```
+
+---
+
+## Error Handling and Exception Safety
+
+### Prefer RAII for cleanup
+
+```cpp
+// ? Good: RAII handles cleanup on exceptions
+void process() {
+    std::vector<int> data = load_data(); // safe cleanup
+    do_work(data);
+}
+```
+
+### Do not throw from destructors
+
+```cpp
+struct File {
+    ~File() noexcept { close(); }
+    void close();
+};
+```
+
+### Use expected results for normal failures
+
+```cpp
+// ? Expected error: use optional or expected
+std::optional<int> parse_int(const std::string& s) {
+    try {
+        return std::stoi(s);
+    } catch (...) {
+        return std::nullopt;
+    }
+}
+```
+
+---
+
+## Concurrency
+
+### Protect shared data
+
+```cpp
+// ? Bad: data race
+int counter = 0;
+void inc() { counter++; }
+
+// ? Good: atomic
+std::atomic<int> counter{0};
+void inc() { counter.fetch_add(1, std::memory_order_relaxed); }
+```
+
+### Use RAII locks
+
+```cpp
+std::mutex mu;
+std::vector<int> data;
+
+void add(int v) {
+    std::lock_guard<std::mutex> lock(mu);
+    data.push_back(v);
+}
+```
+
+---
+
+## Performance and Allocation
+
+### Avoid repeated allocations
+
+```cpp
+// ? Bad: repeated reallocation
+std::vector<int> build(int n) {
+    std::vector<int> out;
+    for (int i = 0; i < n; ++i) {
+        out.push_back(i);
+    }
+    return out;
+}
+
+// ? Good: reserve upfront
+std::vector<int> build(int n) {
+    std::vector<int> out;
+    out.reserve(static_cast<size_t>(n));
+    for (int i = 0; i < n; ++i) {
+        out.push_back(i);
+    }
+    return out;
+}
+```
+
+### String concatenation
+
+```cpp
+// ? Bad: repeated allocation
+std::string join(const std::vector<std::string>& parts) {
+    std::string out;
+    for (const auto& p : parts) {
+        out += p;
+    }
+    return out;
+}
+
+// ? Good: reserve total size
+std::string join(const std::vector<std::string>& parts) {
+    size_t total = 0;
+    for (const auto& p : parts) {
+        total += p.size();
+    }
+    std::string out;
+    out.reserve(total);
+    for (const auto& p : parts) {
+        out += p;
+    }
+    return out;
+}
+```
+
+---
+
+## Templates and Type Safety
+
+### Prefer constrained templates (C++20)
+
+```cpp
+// ? Bad: overly generic
+template <typename T>
+T add(T a, T b) {
+    return a + b;
+}
+
+// ? Good: constrained
+template <typename T>
+requires std::is_integral_v<T>
+T add(T a, T b) {
+    return a + b;
+}
+```
+
+### Use static_assert for invariants
+
+```cpp
+template <typename T>
+struct Packet {
+    static_assert(std::is_trivially_copyable_v<T>,
+        "Packet payload must be trivially copyable");
+    T payload;
+};
+```
+
+---
+
+## Tooling and Build Checks
+
+```bash
+# Warnings
+clang++ -Wall -Wextra -Werror -Wconversion -Wshadow -std=c++20 ...
+
+# Sanitizers (debug builds)
+clang++ -fsanitize=address,undefined -fno-omit-frame-pointer -g ...
+clang++ -fsanitize=thread -fno-omit-frame-pointer -g ...
+
+# Static analysis
+clang-tidy src/*.cpp -- -std=c++20
+
+# Formatting
+clang-format -i src/*.cpp include/*.h
+```
+
+---
+
+## Review Checklist
+
+### Safety and Lifetime
+- [ ] Ownership is explicit (RAII, unique_ptr by default)
+- [ ] No dangling references or views
+- [ ] Rule of 0/3/5 followed for resource-owning types
+- [ ] No raw new/delete in business logic
+- [ ] Destructors are noexcept and do not throw
+
+### API and Design
+- [ ] const-correctness is applied consistently
+- [ ] Constructors are explicit where needed
+- [ ] Override/final used for virtual functions
+- [ ] No object slicing (pass by ref or pointer)
+
+### Concurrency
+- [ ] Shared data is protected (mutex or atomics)
+- [ ] Locking order is consistent
+- [ ] No blocking while holding locks
+
+### Performance
+- [ ] Unnecessary allocations avoided (reserve, move)
+- [ ] Copies avoided in hot paths
+- [ ] Algorithmic complexity is reasonable
+
+### Tooling and Tests
+- [ ] Builds clean with warnings enabled
+- [ ] Sanitizers run on critical code paths
+- [ ] Static analysis (clang-tidy) results are addressed