es-ude / es-ude/OnDeviceTraining
Common.h: add PRINT_WARN + unified assert macros
- Dominant language
- C
- Stars
- 1
- Forks
- 3
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 8
Description
## Context
`src/common/include/Common.h` provides `PRINT_DEBUG` / `PRINT_INFO` / `PRINT_ERROR` (DLEVEL-staged) but no `PRINT_WARN`. There is also no codebase-wide assert wrapper — programming-error checks are scattered across three patterns:
| Pattern | Behavior | Where used |
|---|---|---|
| `PRINT_ERROR("...") + exit(1)` | Always logs (DLEVEL>=1), abort | "Unknown QType!" default branches in switch statements |
| `` `assert(...)` | Debug-only (NDEBUG) check, abort | Effectively never used systematically |
| Bare `PRINT_ERROR` without exit | Just logs | Rare; semantic mismatch (ERROR-named macro for warnings) |
This was surfaced during the #135 design discussion (2026-05-04): factor validation in `scaleOptimizerGradients` wants a "warning, not abort" path, but the codebase has no `PRINT_WARN`. Using `PRINT_ERROR` for warnings is misleading.
## Proposed scope
1. **Add `PRINT_WARN` macro** to `Common.h`, gated on `DLEVEL >= 2` (same level as `PRINT_INFO` but with a distinct color, e.g., yellow). Pattern matches existing `PRINT_*` family.
2. **Add `ASSERT_DEBUG(cond, msg)`** — checks condition; on failure logs via `PRINT_ERROR` and `exit(1)`. Active only when `DLEVEL >= 3` (debug builds). Zero overhead in release.
3. **Add `ASSERT_ALWAYS(cond, msg)`** — same as above but always active. Replaces the current `if (!cond) { PRINT_ERROR(...); exit(1); }` boilerplate.
4. **Migrate existing call sites** to the new macros:
- All ``PRINT_ERROR("Unknown QType!"); exit(1);`` in switch defaults → `ASSERT_ALWAYS(false, "Unknown QType")` or equivalent.
- Programming-error checks added by #135 (e.g., factor validation) → `PRINT_WARN(...)`.
- Other `PRINT_ERROR + exit(1)` patterns audited and converted.
5. **Document conventions** in `docs/CONVENTIONS.md`: when to use which macro (warning vs. always-fatal vs. debug-only-fatal).
## Why separate from #135
- Cross-cutting: touches every existing `PRINT_ERROR + exit(1)` site (estimated 20+ across `src/`).
- Conflicts with parallel work — see memory entry *No repo-wide diffs while Jan parallel*. Better landed in a quiet window.
- Scope is genuinely about codebase consistency, not loss-API semantics. Mixing would balloon the #135 PR.
## Acceptance criteria
- [ ] `PRINT_WARN` exists with a color/level distinct from `PRINT_ERROR`.
- [ ] `ASSERT_DEBUG` and `ASSERT_ALWAYS` exist and replace at least the switch-default pattern across `src/`.
- [ ] All existing ``PRINT_ERROR + exit(1)`` patterns migrated or explicitly justified to remain.
- [ ] CONVENTIONS.md documents the trinity (warn / debug-assert / always-assert) with examples.
- [ ] Existing tests pass; no behavior change in release builds.
## Related
- Surfaced in the #135 brainstorm — `scaleOptimizerGradients` factor validation needed `PRINT_WARN` which didn't exist.
- Touches the `DLEVEL` macro-redefinition warning issue #52 (same header).
Contributor guide
Assessment
This issue has not been assessed yet.