path: root/CHECK_RETURN_ASSESSMENT.md

# 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.