summaryrefslogtreecommitdiff
path: root/CHECK_RETURN_ASSESSMENT.md
diff options
context:
space:
mode:
Diffstat (limited to 'CHECK_RETURN_ASSESSMENT.md')
-rw-r--r--CHECK_RETURN_ASSESSMENT.md326
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.