diff --git a/style.md b/style.md index 70591b0..583d846 100644 --- a/style.md +++ b/style.md @@ -133,11 +133,11 @@ template struct rebind { using type = T*; }; ## File Organization -### Header Files +### Include Organization - Use **`#pragma once`** instead of include guards - **Never `using namespace std`** - always use fully qualified names for clarity and safety -- **Include order:** - 1. Corresponding header file (for .cpp files) +- **Include order** (applies to both headers and source files): + 1. Corresponding header file (for .cpp files only) 2. Standard library headers (alphabetical) 3. Third-party library headers 4. Project headers @@ -164,10 +164,6 @@ std::vector data; std::unique_ptr parser; ``` -### Source Files -- Include corresponding header first -- Follow same include order as headers (see Header Files section above) - --- ## Code Structure @@ -211,23 +207,22 @@ ArenaAllocator(ArenaAllocator &&other) noexcept; ### Template Usage - **Template constraints** using static_assert for better error messages - **SFINAE** or concepts for template specialization -```cpp -template T *construct(Args &&...args) { - static_assert( - std::is_trivially_destructible_v, - "ArenaAllocator::construct requires trivially destructible types."); - // ... -} -``` -### Factory Patterns -- **Static factory methods** for complex construction requiring specific initialization +### Factory Patterns & Ownership +- **Static factory methods** for complex construction requiring shared ownership - **Friend-based factories** for access control when constructor should be private -- **Factory patterns ensure proper ownership semantics** (shared_ptr vs unique_ptr) +- **Ownership guidelines:** + - **unique_ptr** for exclusive ownership (most common case) + - **shared_ptr** only when multiple owners need concurrent access to same object + - **Factory methods return appropriate smart pointer type** based on ownership needs + ```cpp -// Static factory method +// Shared ownership - multiple components need concurrent access auto server = Server::create(config, handler); // Returns shared_ptr +// Exclusive ownership - single owner, transfer via move +auto connection = Connection::createForServer(...); // Returns unique_ptr + // Friend-based factory for access control struct Connection { void appendMessage(std::string_view data); @@ -257,12 +252,8 @@ for (auto &precondition : preconditions_) { ### Ownership & Allocation - **Arena allocators** for request-scoped memory with **STL allocator adapters** (provides ~1ns allocation vs ~20-270ns for malloc) - **String views** pointing to arena-allocated memory for zero-copy operations -- **Prefer unique_ptr** for exclusive ownership -- **shared_ptr only if shared ownership is necessary** - most objects have single owners -- **Factory patterns** for complex construction and ownership control (see Code Structure section for factory patterns) - **STL containers with arena allocators require default construction after arena reset** - `clear()` is not sufficient ```cpp - // STL containers with arena allocators - correct reset pattern std::vector> operations(arena_alloc); // ... use container ... @@ -288,49 +279,48 @@ arena.reset(); // Reset arena memory ## Error Handling -### Error Philosophy -- **Return codes** for expected errors that can be handled -- **Abort for system failures** - If we can't uphold the component's contract, perror/fprintf then abort. If recovery is possible, change the component's contract to allow returning an error code. -- **Error messages are for humans only** - never parse error message strings programmatically -- **Error codes are the contract** - use enums/codes for programmatic error handling +### Error Classification & Response +- **Expected errors** (invalid input, timeouts): Return error codes for programmatic handling +- **System failures** (malloc fail, socket fail): Abort immediately with error message +- **Programming errors** (precondition violations, assertions): Abort immediately + +### Error Contract Design +- **Error codes are the API contract** - use enums for programmatic decisions +- **Error messages are human-readable only** - never parse message strings +- **Consistent error boundaries** - each component defines what it can/cannot recover from +- **Interface precondition violations are undefined behavior** - acceptable to skip checks for performance in hot paths -### Error Boundaries -- **Expected errors**: Invalid user input, network timeouts, file not found - return error codes -- **System failures**: Memory allocation failure, socket creation failure - abort immediately -- **Programming errors**: Assertion failures, null pointer dereference - abort immediately ```cpp enum class ParseResult { Success, InvalidJson, MissingField }; -// Good: Test error codes (part of contract) -auto result = parser.parse(data); -if (result == ParseResult::InvalidJson) { - // Handle programmatically +// System failure - abort immediately +void* memory = malloc(size); +if (!memory) { + std::fprintf(stderr, "ArenaAllocator: Memory allocation failed\n"); + std::abort(); } +// ... use memory, eventually free it -// Bad: Don't test or parse error message strings -// CHECK(parser.get_error() == "Expected '}' at line 5"); // BRITTLE! - -// System resource failures: abort immediately -void ArenaAllocator::allocate() { - void* memory = malloc(size); - if (!memory) { - std::fprintf(stderr, "ArenaAllocator: Failed to allocate memory\n"); - std::abort(); // Process is likely in bad state - } -} +// Programming error - precondition violation (may be omitted for performance) +assert(ptr != nullptr && "Precondition violated: pointer must be non-null"); ``` ### Assertions -- Use **assert()** for debug-time checks that validate program correctness -- **Static assertions** for compile-time validation -- **Standard assert behavior**: Assertions are **enabled by default** and **disabled when `NDEBUG` is defined** -- **Use assertions for programming errors**: Null pointer checks, precondition validation, invariant checking -- **Don't use assertions for expected runtime errors**: Use return codes for recoverable conditions +- **Programming error detection** using standard `assert()` macro +- **Assertion behavior follows C++ standards:** + - **Debug builds**: Assertions active (undefined `NDEBUG`) + - **Release builds**: Assertions removed (defined `NDEBUG`) +- **Test targets override**: Use `-UNDEBUG` to force assertions active in all builds +- **Static assertions** for compile-time validation (always active) + +**Usage guidelines:** +- Use for programming errors: null checks, precondition validation, invariants +- Don't use for expected runtime errors: use return codes instead + ```cpp -// Good: Programming error checks (enabled by default, disabled with NDEBUG) +// Good: Programming error checks assert(current_block_ && "realloc called with non-null ptr but no current block"); assert(size > 0 && "Cannot allocate zero bytes"); -assert(ptr != nullptr && "Invalid pointer passed to realloc"); // Good: Compile-time validation (always enabled) static_assert(std::is_trivially_destructible_v, "Arena requires trivially destructible types"); @@ -339,12 +329,6 @@ static_assert(std::is_trivially_destructible_v, "Arena requires trivially des // assert(file_exists(path)); // File might legitimately not exist - use return code instead ``` -**Build Configuration:** -- **Debug builds**: `cmake -DCMAKE_BUILD_TYPE=Debug` → assertions **enabled** (default behavior) -- **Release builds**: `cmake -DCMAKE_BUILD_TYPE=Release` → assertions **disabled** (defines `NDEBUG`) -- **Test targets**: Always have assertions **enabled** using `-UNDEBUG` pattern (see Build Integration section) -- **Testing**: Test in both debug and release builds to catch assertion failures in all configurations - --- ## Documentation @@ -435,17 +419,15 @@ TEST_CASE("Server accepts connections") { // TEST_CASE("Server creates epoll instance") { /* implementation detail */ } ``` -### Test Synchronization -- **NEVER use timeouts** or sleep-based synchronization +### Test Synchronization (Authoritative Rules) +- **ABSOLUTELY NEVER use timeouts** (`sleep_for`, `wait_for`, etc.) - **Deterministic synchronization only:** - - Blocking I/O operations - - `condition_variable.wait()` (no timeout variant) + - Blocking I/O (naturally waits for completion) + - `condition_variable.wait()` without timeout - `std::latch`, `std::barrier`, futures/promises - - RAII guards and resource management +- **Force concurrent execution** using `std::latch` to synchronize thread startup +- **Tests should pass or hang** - no flaky timeout behavior -### Multithreading Test Correctness -- **Force concurrent execution** - Thread creation takes time, so work often completes sequentially before threads start -- **Use std::latch to synchronize thread startup** - Ensures all threads begin racing simultaneously ```cpp // BAD: Race likely over before threads start std::atomic counter{0}; @@ -468,33 +450,20 @@ for (int i = 0; i < 4; ++i) { ## Build Integration -### CMake Integration -- **Generated code** (gperf hash tables) in build directory -- **Ninja** as the preferred generator -- **Export compile commands** for tooling support - -**Build Types:** +### Build Configuration ```bash -# Debug build (assertions enabled by default, optimizations off) +# Debug: assertions on, optimizations off cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -# Release build (assertions disabled, optimizations on) +# Release: assertions off, optimizations on cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON ``` -**Testing and Development:** -- **Test targets always have assertions enabled** - even in release builds, test targets use `-UNDEBUG` to ensure assertions are checked -- **Production builds have assertions disabled** - the main `weaseldb` executable follows standard build type behavior -- **Use Release builds for performance testing** and production deployment -- **This ensures tests catch assertion failures** regardless of build configuration +**Test Target Pattern:** +- Production targets follow build type (assertions off in Release) +- Test targets use `-UNDEBUG` to force assertions on in all builds +- Ensures consistent test validation regardless of build type -### Test Assertion Pattern (-UNDEBUG) - -**Problem**: Release builds define `NDEBUG` which disables assertions, but tests should always validate assertions to catch programming errors. - -**Solution**: Use `-UNDEBUG` compiler flag for test targets to undefine `NDEBUG` and re-enable assertions. - -**CMake Implementation:** ```cmake # Test target with assertions always enabled add_executable(test_example tests/test_example.cpp src/example.cpp) @@ -506,12 +475,6 @@ add_executable(example src/example.cpp src/main.cpp) # No -UNDEBUG → assertions disabled in Release, enabled in Debug ``` -**Benefits:** -- **Consistent test behavior**: Tests validate assertions in both Debug and Release builds -- **Production performance**: Production binaries maintain optimized release performance -- **Early error detection**: Catch assertion failures during CI/CD regardless of build configuration -- **Build type flexibility**: Can use Release builds for performance profiling while still testing assertions - ### Code Generation - **gperf** for perfect hash table generation - **Build-time generation** of token lookup tables