refactor(slf): extract FormulaHelper to its own TU (isolate exprtk)#57
refactor(slf): extract FormulaHelper to its own TU (isolate exprtk)#57alandtse wants to merge 1 commit into
Conversation
Move the FormulaParams enum, kFormulaVars table, and the exprtk-backed
FormulaHelper out of ShadowCasterManager into FormulaHelper.{h,cpp}. The
header uses a void* pimpl so it stays exprtk-free; FormulaHelper.cpp is the
single TU that compiles the (huge) exprtk header. ShadowCasterManager.cpp is
now exprtk-free (the score/redraw formulas still call FormulaHelper::SetParam
etc. unchanged); the formula-editor UI keeps using kFormulaVars from the
header. Pure code move -- no behavior change. Plugin builds clean.
Also adds an opt-in cpp_tests_slow target (-DBUILD_CPP_SLOW_TESTS=ON, OFF by
default so the fast suite / PR gate is unaffected) with test_formulahelper.cpp
covering Parse/Calculate/Validate/Reparse, param binding, and error handling.
KNOWN ISSUE: cpp_tests_slow builds but currently registers 0 Catch2 test
cases under the local MSVC 14.50 toolchain (a trivial probe TEST_CASE in the
main TU also fails to register), so the tests don't yet execute -- a
target-level Catch2 registration problem to resolve, independent of the
extraction (which is plugin-validated) and the test code.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No actionable suggestions for changed features. |
|
Abandoning this approach. The opt-in slow-test target registers 0 Catch2 cases under local MSVC 14.50 (a target-level registration issue), so per direction the dead test is removed. Restoring ShadowCasterManager (un-extracting FormulaHelper) per request. Note on /bigobj: verified empirically that it is NOT avoidable by restoring SCM — exprtk alone exceeds the COFF section limit under MSVC 14.50 (C1128 at exprtk.hpp:12094 even in a standalone FormulaHelper.cpp). So SCM+exprtk and FormulaHelper.cpp+exprtk both require /bigobj locally; CI's VS2022 doesn't need it. /bigobj stays in #56 as a harmless enabler for local VS2026 builds. |
|
✅ A pre-release build is available for this PR: |
1 participant
Summary
Completes the Tier-2 plan: extracts FormulaHelper (the exprtk-backed score / redraw-interval / redraw-budget formula evaluator) out of
ShadowCasterManagerinto its own TU, with an opt-in slow-tests target for it (your chosen approach — keeps the fast suite/CI gate fast).Extraction (pure code move, plugin-validated)
Features/LightLimitFix/FormulaHelper.{h,cpp}: theFormulaParamsenum, thekFormulaVarstable, andFormulaHelper(Parse/Calculate/Reparse/Validate/SetParam). The header uses avoid*pimpl so it stays exprtk-free;FormulaHelper.cppis the one TU that compiles the huge exprtk header.ShadowCasterManager.cppis now exprtk-free — the scoring loop still callsFormulaHelper::SetParam(...)unchanged, and the formula-editor UI keeps usingkFormulaVarsfrom the header.CommunityShaders) builds clean with the extraction.Opt-in slow-tests target
cpp_tests_slow(-DBUILD_CPP_SLOW_TESTS=ON, OFF by default) compilesFormulaHelper.cpp+ exprtk +test_formulahelper.cpp(Parse/Calculate/Validate/Reparse, param binding, malformed/unknown-variable handling). The defaultcpp_testsand the PR gate are untouched, so the fast loop stays fast.cpp_tests_slowbuilds and links but currently registers 0 Catch2 test cases under the local MSVC 14.50 / VS2026 toolchain — a trivial probeTEST_CASEin the main TU also fails to register, so this is a target-level Catch2 registration problem (likely a CRT-runtime/registry-singleton interaction in the minimal standalone target), not an issue with the extraction or the test code. The fastcpp_testsregisters fine, so the test logic is sound. The tests don't execute until this harness issue is resolved.Suggested split: the extraction is clean, validated, and independently valuable — it can merge on its own. The slow-test target is scaffolding pending the registration fix.