weaselab/weaseldb
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Slim down style.md

Browse Source
This commit is contained in:
Andrew Noyes
2025-08-23 13:33:17 -04:00
parent 3712622d11
commit f576e96b5d
1 changed files with 57 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

151
style.md
View File

@@ -133,11 +133,11 @@ template <typename T> struct rebind { using type = T*; };
## File Organization ## File Organization
### Header Files ### Include Organization
- Use **`#pragma once`** instead of include guards - Use **`#pragma once`** instead of include guards
- **Never `using namespace std`** - always use fully qualified names for clarity and safety - **Never `using namespace std`** - always use fully qualified names for clarity and safety
- **Include order:** - **Include order** (applies to both headers and source files):
1. Corresponding header file (for .cpp files) 1. Corresponding header file (for .cpp files only)
2. Standard library headers (alphabetical) 2. Standard library headers (alphabetical)
3. Third-party library headers 3. Third-party library headers
4. Project headers 4. Project headers
@@ -164,10 +164,6 @@ std::vector<int> data;
std::unique_ptr<Parser> parser; std::unique_ptr<Parser> parser;
``` ```
### Source Files
- Include corresponding header first
- Follow same include order as headers (see Header Files section above)
--- ---
## Code Structure ## Code Structure
@@ -211,23 +207,22 @@ ArenaAllocator(ArenaAllocator &&other) noexcept;
### Template Usage ### Template Usage
- **Template constraints** using static_assert for better error messages - **Template constraints** using static_assert for better error messages
- **SFINAE** or concepts for template specialization - **SFINAE** or concepts for template specialization
```cpp
template <typename T> T *construct(Args &&...args) {
static_assert(
std::is_trivially_destructible_v<T>,
"ArenaAllocator::construct requires trivially destructible types.");
// ...
}
```
### Factory Patterns ### Factory Patterns & Ownership
- **Static factory methods** for complex construction requiring specific initialization - **Static factory methods** for complex construction requiring shared ownership
- **Friend-based factories** for access control when constructor should be private - **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 ```cpp
// Static factory method // Shared ownership - multiple components need concurrent access
auto server = Server::create(config, handler); // Returns shared_ptr 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 // Friend-based factory for access control
struct Connection { struct Connection {
void appendMessage(std::string_view data); void appendMessage(std::string_view data);
@@ -257,12 +252,8 @@ for (auto &precondition : preconditions_) {
### Ownership & Allocation ### Ownership & Allocation
- **Arena allocators** for request-scoped memory with **STL allocator adapters** (provides ~1ns allocation vs ~20-270ns for malloc) - **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 - **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 - **STL containers with arena allocators require default construction after arena reset** - `clear()` is not sufficient
```cpp ```cpp
// STL containers with arena allocators - correct reset pattern // STL containers with arena allocators - correct reset pattern
std::vector<Operation, ArenaStlAllocator<Operation>> operations(arena_alloc); std::vector<Operation, ArenaStlAllocator<Operation>> operations(arena_alloc);
// ... use container ... // ... use container ...
@@ -288,49 +279,48 @@ arena.reset(); // Reset arena memory
## Error Handling ## Error Handling
### Error Philosophy ### Error Classification & Response
- **Return codes** for expected errors that can be handled - **Expected errors** (invalid input, timeouts): Return error codes for programmatic handling
- **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. - **System failures** (malloc fail, socket fail): Abort immediately with error message
- **Error messages are for humans only** - never parse error message strings programmatically - **Programming errors** (precondition violations, assertions): Abort immediately
- **Error codes are the contract** - use enums/codes for programmatic error handling
### 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 ```cpp
enum class ParseResult { Success, InvalidJson, MissingField }; enum class ParseResult { Success, InvalidJson, MissingField };
// Good: Test error codes (part of contract) // System failure - abort immediately
auto result = parser.parse(data); void* memory = malloc(size);
if (result == ParseResult::InvalidJson) { if (!memory) {
// Handle programmatically std::fprintf(stderr, "ArenaAllocator: Memory allocation failed\n");
std::abort();
} }
// ... use memory, eventually free it
// Bad: Don't test or parse error message strings // Programming error - precondition violation (may be omitted for performance)
// CHECK(parser.get_error() == "Expected '}' at line 5"); // BRITTLE! assert(ptr != nullptr && "Precondition violated: pointer must be non-null");
// 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
}
}
``` ```
### Assertions ### Assertions
- Use **assert()** for debug-time checks that validate program correctness - **Programming error detection** using standard `assert()` macro
- **Static assertions** for compile-time validation - **Assertion behavior follows C++ standards:**
- **Standard assert behavior**: Assertions are **enabled by default** and **disabled when `NDEBUG` is defined** - **Debug builds**: Assertions active (undefined `NDEBUG`)
- **Use assertions for programming errors**: Null pointer checks, precondition validation, invariant checking - **Release builds**: Assertions removed (defined `NDEBUG`)
- **Don't use assertions for expected runtime errors**: Use return codes for recoverable conditions - **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 ```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(current_block_ && "realloc called with non-null ptr but no current block");
assert(size > 0 && "Cannot allocate zero bytes"); assert(size > 0 && "Cannot allocate zero bytes");
assert(ptr != nullptr && "Invalid pointer passed to realloc");
// Good: Compile-time validation (always enabled) // Good: Compile-time validation (always enabled)
static_assert(std::is_trivially_destructible_v<T>, "Arena requires trivially destructible types"); static_assert(std::is_trivially_destructible_v<T>, "Arena requires trivially destructible types");
@@ -339,12 +329,6 @@ static_assert(std::is_trivially_destructible_v<T>, "Arena requires trivially des
// assert(file_exists(path)); // File might legitimately not exist - use return code instead // 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 ## Documentation
@@ -435,17 +419,15 @@ TEST_CASE("Server accepts connections") {
// TEST_CASE("Server creates epoll instance") { /* implementation detail */ } // TEST_CASE("Server creates epoll instance") { /* implementation detail */ }
``` ```
### Test Synchronization ### Test Synchronization (Authoritative Rules)
- **NEVER use timeouts** or sleep-based synchronization - **ABSOLUTELY NEVER use timeouts** (`sleep_for`, `wait_for`, etc.)
- **Deterministic synchronization only:** - **Deterministic synchronization only:**
- Blocking I/O operations - Blocking I/O (naturally waits for completion)
- `condition_variable.wait()` (no timeout variant) - `condition_variable.wait()` without timeout
- `std::latch`, `std::barrier`, futures/promises - `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 ```cpp
// BAD: Race likely over before threads start // BAD: Race likely over before threads start
std::atomic<int> counter{0}; std::atomic<int> counter{0};
@@ -468,33 +450,20 @@ for (int i = 0; i < 4; ++i) {
## Build Integration ## Build Integration
### CMake Integration ### Build Configuration
- **Generated code** (gperf hash tables) in build directory
- **Ninja** as the preferred generator
- **Export compile commands** for tooling support
**Build Types:**
```bash ```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 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 cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
``` ```
**Testing and Development:** **Test Target Pattern:**
- **Test targets always have assertions enabled** - even in release builds, test targets use `-UNDEBUG` to ensure assertions are checked - Production targets follow build type (assertions off in Release)
- **Production builds have assertions disabled** - the main `weaseldb` executable follows standard build type behavior - Test targets use `-UNDEBUG` to force assertions on in all builds
- **Use Release builds for performance testing** and production deployment - Ensures consistent test validation regardless of build type
- **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 ```cmake
# Test target with assertions always enabled # Test target with assertions always enabled
add_executable(test_example tests/test_example.cpp src/example.cpp) 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 # 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 ### Code Generation
- **gperf** for perfect hash table generation - **gperf** for perfect hash table generation
- **Build-time generation** of token lookup tables - **Build-time generation** of token lookup tables