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, 326 insertions, 0 deletions
diff --git a/CHECK_RETURN_ASSESSMENT.md b/CHECK_RETURN_ASSESSMENT.md
new file mode 100644
index 0000000..0af818f
--- /dev/null
+++ b/CHECK_RETURN_ASSESSMENT.md
@@ -0,0 +1,326 @@
+# 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.