diff options
Diffstat (limited to 'CHECK_RETURN_ASSESSMENT.md')
| -rw-r--r-- | CHECK_RETURN_ASSESSMENT.md | 326 |
1 files changed, 0 insertions, 326 deletions
diff --git a/CHECK_RETURN_ASSESSMENT.md b/CHECK_RETURN_ASSESSMENT.md deleted file mode 100644 index 0af818f..0000000 --- a/CHECK_RETURN_ASSESSMENT.md +++ /dev/null @@ -1,326 +0,0 @@ -# CHECK_AND_RETURN Macro Assessment & Plan - -## Current Situation - -**Problem:** Repetitive error handling pattern throughout codebase: -```cpp -if (condition) { - fprintf(stderr, "Error: ...\n", ...); - // Optional cleanup - return error_value; -} -``` - -This pattern appears in: -- Input validation (command-line args, file I/O) -- Resource allocation failures -- Runtime configuration errors -- API parameter validation - -**Unlike FATAL_XXX:** These are recoverable errors - caller can handle them. - -## Assessment - -### Files with Error Handling Patterns - -Found in: -- `src/util/asset_manager.cc` - 3+ instances (nullptr returns) -- `src/test_demo.cc` - 2+ instances (return 1) -- `src/gpu/texture_manager.cc` - 1 instance -- Test files (webgpu_test_fixture.cc, etc.) - -### Common Patterns - -#### Pattern 1: Return nullptr on error -```cpp -if (!valid) { - fprintf(stderr, "Error: Invalid input: %s\n", name); - if (out_size) *out_size = 0; - return nullptr; -} -``` - -#### Pattern 2: Return error code -```cpp -if (arg_invalid) { - fprintf(stderr, "Error: Unknown option '%s'\n", option); - print_usage(argv[0]); - return 1; -} -``` - -#### Pattern 3: Return false on failure -```cpp -if (!initialized) { - fprintf(stderr, "Error: Not initialized\n"); - return false; -} -``` - -#### Pattern 4: Warning (continue execution) -```cpp -if (non_critical) { - fprintf(stderr, "Warning: %s\n", msg); - // Continue execution -} -``` - -## Design Requirements - -### 1. Multiple Return Types -- `nullptr` (most common for pointer functions) -- `false` (for bool functions) -- `-1` or error codes (for int functions) -- `{}` or default values (for struct functions) - -### 2. Optional Cleanup -- Some cases need cleanup before return (e.g., `delete[]`) -- Some need to set output parameters (e.g., `*out_size = 0`) - -### 3. Stripping Behavior -- **STRIP_ALL:** Keep error checking, strip messages (save size) -- **FINAL_STRIP:** Strip everything (optional - may keep checks) - -Unlike FATAL_XXX which always abort, CHECK_RETURN should preserve control flow even when stripped. - -### 4. Debug Builds -- Print full error messages with context -- Optional file:line info (like FATAL_XXX) - -## Proposed Macro Design - -### Option A: Single Macro with Return Value - -```cpp -// Usage: -CHECK_RETURN(ptr == nullptr, nullptr, "Asset not found: %s", name); -CHECK_RETURN(argc < 2, 1, "Too few arguments"); -CHECK_RETURN(!initialized, false, "Not initialized"); - -// Expands to: -#if !defined(STRIP_ALL) - if (ptr == nullptr) { - fprintf(stderr, "Error: Asset not found: %s [file:line]\n", name); - return nullptr; - } -#else - if (ptr == nullptr) return nullptr; // Silent check -#endif -``` - -**Pros:** Simple, covers most cases -**Cons:** No cleanup before return, rigid pattern - -### Option B: Separate Macros per Return Type - -```cpp -CHECK_RETURN_NULL(cond, msg, ...) // returns nullptr -CHECK_RETURN_FALSE(cond, msg, ...) // returns false -CHECK_RETURN_ERROR(cond, msg, ...) // returns -1 -CHECK_RETURN_CODE(cond, code, msg, ...) // returns custom code - -// Usage: -CHECK_RETURN_NULL(ptr == nullptr, "Asset not found: %s", name); -CHECK_RETURN_ERROR(fd < 0, "Failed to open file: %s", path); -CHECK_RETURN_CODE(invalid_arg, 1, "Unknown option: %s", arg); -``` - -**Pros:** Type-safe, explicit intent -**Cons:** More macros, more verbose - -### Option C: Block-Based with Cleanup - -```cpp -CHECK_AND_RETURN_IF(condition, return_value) { - // Cleanup code here (optional) - delete[] buffer; - *out_size = 0; - ERROR_MSG("Failed: %s", reason); -} - -// Expands to: -#if !defined(STRIP_ALL) - if (condition) { - delete[] buffer; - *out_size = 0; - fprintf(stderr, "Error: Failed: %s [file:line]\n", reason); - return return_value; - } -#endif -``` - -**Pros:** Flexible, allows cleanup -**Cons:** More complex, harder to read - -### Option D: Hybrid (Recommended) - -```cpp -// Simple cases (90%) -CHECK_RETURN_IF(cond, retval, msg, ...) - -// Complex cases with cleanup (10%) -CHECK_RETURN_BEGIN(cond, retval) - delete[] buffer; - *out_size = 0; - ERROR_MSG("Failed: %s", reason); -CHECK_RETURN_END - -// Warning messages (non-fatal) -WARN_IF(cond, msg, ...) -``` - -**Pros:** Covers all cases, clean separation -**Cons:** Two patterns to learn - -## Recommendation: Option D (Hybrid) - -### Macro Definitions - -```cpp -// ============================================================================ -// Simple error check with immediate return -// ============================================================================ -#if !defined(STRIP_ALL) - #define CHECK_RETURN_IF(cond, retval, ...) \ - do { \ - if (cond) { \ - fprintf(stderr, "Error: " __VA_ARGS__); \ - fprintf(stderr, " [%s:%d]\n", __FILE__, __LINE__); \ - return retval; \ - } \ - } while (0) -#else - #define CHECK_RETURN_IF(cond, retval, ...) \ - do { if (cond) return retval; } while (0) -#endif - -// ============================================================================ -// Block-based error check with cleanup -// ============================================================================ -#if !defined(STRIP_ALL) - #define CHECK_RETURN_BEGIN(cond, retval) if (cond) { - #define ERROR_MSG(...) fprintf(stderr, "Error: " __VA_ARGS__); \ - fprintf(stderr, " [%s:%d]\n", __FILE__, __LINE__) - #define CHECK_RETURN_END return retval; } -#else - #define CHECK_RETURN_BEGIN(cond, retval) if (cond) { - #define ERROR_MSG(...) ((void)0) - #define CHECK_RETURN_END return retval; } -#endif - -// ============================================================================ -// Warning message (non-fatal, continue execution) -// ============================================================================ -#if !defined(STRIP_ALL) - #define WARN_IF(cond, ...) \ - do { \ - if (cond) { \ - fprintf(stderr, "Warning: " __VA_ARGS__); \ - fprintf(stderr, "\n"); \ - } \ - } while (0) -#else - #define WARN_IF(cond, ...) ((void)0) -#endif -``` - -## Usage Examples - -### Before (asset_manager.cc) -```cpp -if (proc_gen_func_ptr == nullptr) { - fprintf(stderr, "Error: Unknown procedural function at runtime: %s\n", - source_record.proc_func_name_str); - if (out_size) *out_size = 0; - return nullptr; -} -``` - -### After (simple version) -```cpp -CHECK_RETURN_BEGIN(proc_gen_func_ptr == nullptr, nullptr) - if (out_size) *out_size = 0; - ERROR_MSG("Unknown procedural function: %s", source_record.proc_func_name_str); -CHECK_RETURN_END -``` - -### Before (test_demo.cc) -```cpp -if (strcmp(argv[i], "--log-peaks") == 0) { - if (i + 1 < argc) { - log_peaks_file = argv[++i]; - } else { - fprintf(stderr, "Error: --log-peaks requires a filename argument\n\n"); - print_usage(argv[0]); - return 1; - } -} -``` - -### After -```cpp -if (strcmp(argv[i], "--log-peaks") == 0) { - CHECK_RETURN_BEGIN(i + 1 >= argc, 1) - print_usage(argv[0]); - ERROR_MSG("--log-peaks requires a filename argument"); - CHECK_RETURN_END - log_peaks_file = argv[++i]; -} -``` - -## Size Impact - -### STRIP_ALL Build -- Error messages stripped (~50-100 bytes per call site) -- Control flow preserved (if-return kept) -- Estimated savings: ~1-2KB for typical project - -### FINAL_STRIP Build (Optional) -- Could strip checks entirely for performance -- NOT recommended for input validation -- Recommended only for internal invariants - -## Implementation Plan - -### Phase 1: Create Header -- [ ] Create `src/util/check_return.h` -- [ ] Define macros (CHECK_RETURN_IF, CHECK_RETURN_BEGIN/END, WARN_IF) -- [ ] Add comprehensive documentation -- [ ] Add usage examples - -### Phase 2: Apply to Key Files -- [ ] `src/util/asset_manager.cc` (3 call sites) -- [ ] `src/test_demo.cc` (2 call sites) -- [ ] `src/gpu/texture_manager.cc` (1 call site) -- [ ] Test files as needed - -### Phase 3: Testing -- [ ] Verify normal build (messages appear) -- [ ] Verify STRIP_ALL build (messages stripped, checks remain) -- [ ] Verify all tests pass -- [ ] Check binary size impact - -### Phase 4: Documentation -- [ ] Update CONTRIBUTING.md with new patterns -- [ ] Add to HOWTO.md "Error Handling" section -- [ ] Update AI_RULES.md if needed - -## Alternative Considered: Don't Do It - -**Argument:** Only ~10-15 call sites, not worth adding complexity. - -**Counter-argument:** -- Improves consistency across codebase -- Reduces boilerplate (3-5 lines → 1-2 lines) -- Matches existing FATAL_XXX pattern (developers already familiar) -- Easy to strip for size optimization -- Low maintenance burden (simple macros) - -## Decision Point - -**Proceed?** User feedback needed: -1. Is the benefit worth the added complexity? -2. Should FINAL_STRIP strip these checks entirely? -3. Prefer Option A (simple), D (hybrid), or alternative? - -**If approved:** Implement Phase 1 first, review, then proceed with Phase 2-4. |
