Repository navigation
fix: make AlgorithmSvc usable - #39
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The review surfaced no outstanding defects or unresolved concerns, leaving nothing that would block approval.
No open issues 🎉
What changed in this PR
This PR rewrites AlgorithmSvc (a factory service that creates Algorithm instances indexed by algorithm signature and demangled type name) so it actually compiles and works, and adds a dedicated test. Previously the service could not have any users because it lacked the service boilerplate/init(), misused algorithm_type as a value, inverted available(), stored dangling string_view keys, mislocked a function-local static lock_guard, and returned raw pointers as unique_ptr. The fix makes the service usable within the broader stacked effort to get core fully tested.
Changes:
- Rewrites
AlgorithmSvcwith owningstd::stringkeys + heterogeneous lookup, a member mutex,try_emplacefor idempotent registration,unique_ptr<AlgorithmBase>factories, demangled type names in errors, and theALGORITHMS_DEFINE_LOGGED_SERVICE/init()boilerplate. - Adds
core/tests/algorithm_svc.cppcovering registration (incl. duplicates), use-before-init()rejection, listing by type, instance creation/execution, and rejection of unknown names/types and empty names.
| File | Description |
|---|---|
core/include/algorithms/algorithm.h |
Rewrites AlgorithmSvc to compile and function correctly: service boilerplate + init(), type-correct algorithm_type usage, member mutex, owning string keys with transparent lookup, and proper unique_ptr ownership transfer. |
core/tests/algorithm_svc.cpp |
New Catch2 test exercising the full service lifecycle (register/list/get/process and error cases) in a single process, auto-registered via the existing test CMake GLOB. |
Review effort: Balanced
d781b8b to
35fe9e7
Compare
AlgorithmSvc did not compile once used: it lacked the logged service boilerplate and init(), used Algo::algorithm_type as a value, inverted available(), stored string_view keys of temporary strings, called std::for_each with four arguments, and returned raw pointers as unique_ptr. Its static lock_guard also never released its mutex. Rewrite it with owning name keys (with heterogeneous lookup), a member mutex guarding the factories, std::unique_ptr factories, and ls() returning std::string names. Error messages use demangled type names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
20d698e to
d14c20f
Compare
Stacked on #38.
Summary
AlgorithmSvcdid not compile once used, so it could not have any users. It:ALGORITHMS_DEFINE_LOGGED_SERVICE(no default constructor orkName) andinit();Algo::algorithm_typeas a value, and invertedavailable();std::string_viewkeys of temporary strings;std::for_eachwith four arguments inls();std::unique_ptr;std::lock_guard, which never releases its mutex.The rewrite keeps the design (factories indexed by algorithm signature, then by demangled type name) with:
std::stringname keys with heterogeneous lookup bystd::string_view;std::unique_ptr<AlgorithmBase>;ls()returningstd::vector<std::string>;Tests
New
core/tests/algorithm_svc.cpp: registration (including duplicates), use beforeServiceSvc::init(), listing by algorithm type, creating and running instances, and rejecting unknown names, unknown types and empty names.With the whole stack (#34 to this PR), line coverage of
coreis 99.5% (gcov/lcov). The two uncovered lines are an unreachable demangling error branch and a gcov artifact on a closing brace.Test plan
eic_xl:nightly: 85/85 tests pass🤖 Generated with Claude Code