Many style updates around asserts, aborts, and errors

style.md

@@ -47,7 +47,7 @@ This document describes the C++ coding style used in the WeaselDB project. These
 - **Strive for 0% CPU usage when idle** - avoid polling, busy waiting, or unnecessary background activity
 - Use **inline functions** for performance-critical code (e.g., `allocate_raw`)
 - **Zero-copy operations** with `std::string_view` over string copying
-- **Arena allocation** for efficient memory management
+- **Arena allocation** for efficient memory management (see Memory Management section for details)
 
 ### Complexity Control
 - **Encapsulation is the main tool for controlling complexity**
@@ -126,7 +126,7 @@ private:
 - **PascalCase** for template type parameters
 ```cpp
 template <typename T, typename... Args>
-template <typename U> struct rebind {};
+template <typename T> struct rebind { using type = T*; };
 ```
 
 ---
@@ -166,18 +166,7 @@ std::unique_ptr<Parser> parser;
 
 ### Source Files
 - Include corresponding header first
-- Follow same include order as headers
-```cpp
-#include "json_commit_request_parser.hpp"
-
-#include <cassert>
-#include <charconv>
-#include <cstring>
-
-#include <simdutf.h>
-
-#include "json_token_enum.hpp"
-```
+- Follow same include order as headers (see Header Files section above)
 
 ---
 
@@ -231,6 +220,23 @@ template <typename T> T *construct(Args &&...args) {
 }
 ```
 
+### Factory Patterns
+- **Static factory methods** for complex construction requiring specific initialization
+- **Friend-based factories** for access control when constructor should be private
+- **Factory patterns ensure proper ownership semantics** (shared_ptr vs unique_ptr)
+```cpp
+// Static factory method
+auto server = Server::create(config, handler);  // Returns shared_ptr
+
+// Friend-based factory for access control
+struct Connection {
+  void appendMessage(std::string_view data);
+private:
+  Connection(/* args */);  // Private constructor
+  friend struct Server;   // Only Server can construct
+};
+```
+
 ### Control Flow
 - **Early returns** to reduce nesting
 - **Range-based for loops** when possible
@@ -249,30 +255,13 @@ for (auto &precondition : preconditions_) {
 ## Memory Management
 
 ### Ownership & Allocation
-- **Arena allocators** for request-scoped memory with **STL allocator adapters**
-- **String views** pointing to arena-allocated memory for zero-copy
+- **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
+- **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
-// Static factory methods for complex objects requiring specific initialization
-auto server = Server::create(config, handler);  // Ensures shared_ptr semantics
-Block *block = Block::create(size, prev);       // Custom allocation + setup
-
-// Friend-based factories for access control
-struct Connection {
-  // Public interface first
-  void appendMessage(std::string_view data);
-  bool writeBytes();
-
-private:
-  Connection(/* args */);  // Private constructor
-  friend struct Server;   // Only Server can construct Connections
-};
-
-// Usage in Server
-auto conn = std::unique_ptr<Connection>(new Connection(args));
 
 // STL containers with arena allocators - correct reset pattern
 std::vector<Operation, ArenaStlAllocator<Operation>> operations(arena_alloc);
@@ -299,11 +288,16 @@ arena.reset();  // Reset arena memory
 
 ## Error Handling
 
-### Error Reporting
-- **Return codes** for expected errors
-- **Avoid exceptions** - If we can't uphold the component's contract, perror/fprintf then abort. If we want to try to recover, change the component's contract to allow returning an error code.
+### 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 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 };
 
@@ -320,20 +314,37 @@ if (result == ParseResult::InvalidJson) {
 void ArenaAllocator::allocate() {
   void* memory = malloc(size);
   if (!memory) {
-    perror("malloc");
+    std::fprintf(stderr, "ArenaAllocator: Failed to allocate memory\n");
     std::abort();  // Process is likely in bad state
   }
 }
 ```
 
 ### Assertions
-- Use **assert()** for debug-time checks
+- 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
 ```cpp
+// Good: Programming error checks (enabled by default, disabled with NDEBUG)
 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<T>, "Arena requires trivially destructible types");
+
+// Bad: Don't use assert for expected runtime errors
+// 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
@@ -407,6 +418,7 @@ TEST_CASE("ArenaAllocator basic allocation") {
 - **Avoid testing private methods directly** - if private functionality needs testing, consider if it should be public or extracted
 - **Both integration and unit tests** - test components in isolation and working together
 - **Prefer fakes to mocks** - use real implementations for internal components, fake external dependencies
+- **Always enable assertions in tests** - use `-UNDEBUG` pattern to ensure assertions are checked (see Build Integration section)
 ```cpp
 // Good: Testing through public API
 TEST_CASE("Server accepts connections") {
@@ -460,10 +472,46 @@ for (int i = 0; i < 4; ++i) {
 - **Generated code** (gperf hash tables) in build directory
 - **Ninja** as the preferred generator
 - **Export compile commands** for tooling support
+
+**Build Types:**
 ```bash
-cmake .. -G Ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
+# Debug build (assertions enabled by default, optimizations off)
+cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
+
+# Release build (assertions disabled, 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 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)
+target_link_libraries(test_example doctest::doctest)
+target_compile_options(test_example PRIVATE -UNDEBUG)  # Always enable assertions
+
+# Production target follows build type
+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