diff --git a/reference/c.md b/reference/c.md new file mode 100644 index 0000000..cfd31ed --- /dev/null +++ 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 diff --git a/reference/cpp.md b/reference/cpp.md new file mode 100644 index 0000000..58743f6 --- /dev/null +++ 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 make_foo() { + auto foo = std::make_unique(); + if (!foo->Init()) { + return {}; + } + return foo; +} +``` + +### Wrap C resources + +```cpp +// ? Good: wrap FILE* with unique_ptr +using FilePtr = std::unique_ptr; + +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 make_task() { + int value = 42; + return [&]() { use(value); }; // dangling +} + +// ? Good: capture by value +std::function 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 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 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 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 counter{0}; +void inc() { counter.fetch_add(1, std::memory_order_relaxed); } +``` + +### Use RAII locks + +```cpp +std::mutex mu; +std::vector data; + +void add(int v) { + std::lock_guard lock(mu); + data.push_back(v); +} +``` + +--- + +## Performance and Allocation + +### Avoid repeated allocations + +```cpp +// ? Bad: repeated reallocation +std::vector build(int n) { + std::vector out; + for (int i = 0; i < n; ++i) { + out.push_back(i); + } + return out; +} + +// ? Good: reserve upfront +std::vector build(int n) { + std::vector out; + out.reserve(static_cast(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& parts) { + std::string out; + for (const auto& p : parts) { + out += p; + } + return out; +} + +// ? Good: reserve total size +std::string join(const std::vector& 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 +T add(T a, T b) { + return a + b; +} + +// ? Good: constrained +template +requires std::is_integral_v +T add(T a, T b) { + return a + b; +} +``` + +### Use static_assert for invariants + +```cpp +template +struct Packet { + static_assert(std::is_trivially_copyable_v, + "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