From d14c20f9c30a49aefc9a545ad1bb09d4844433e7 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Wed, 28 Oct 2026 17:20:30 -0500 Subject: [PATCH] fix: make AlgorithmSvc usable 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 --- core/include/algorithms/algorithm.h | 73 ++++++++++--------- core/tests/algorithm_svc.cpp | 106 ++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 35 deletions(-) create mode 100644 core/tests/algorithm_svc.cpp diff --git a/core/include/algorithms/algorithm.h b/core/include/algorithms/algorithm.h index 9b5ba0d..670ee98 100644 --- a/core/include/algorithms/algorithm.h +++ b/core/include/algorithms/algorithm.h @@ -30,10 +30,13 @@ #pragma once #include +#include #include +#include #include #include #include +#include #include #include #include @@ -105,64 +108,63 @@ template class Algorithm : public AlgorithmB // compile-time code needed. class AlgorithmSvc : public LoggedService { public: + void init() {} + // Add a new factory for an algorithm with a specified type. The full demangled type name // will be used as identifier for the factory template void add() { - static std::mutex m; - static std::lock_guard lock{m}; - auto type = Algo::algorithm_type; - std::type_index name = detail::demangledName(); - if (!available()) { - m_factories[type] = {}; - } - if (available(name)) { - return; // do nothing, already there - } - m_factories[type].emplace({name, []() { return static_cast(new Algo()); }}); + using AlgoType = typename Algo::algorithm_type; + std::lock_guard lock{m_mutex}; + auto& factories = m_factories[typeid(AlgoType)]; + // do nothing if already there + factories.try_emplace(detail::demangledName(), + []() -> std::unique_ptr { return std::make_unique(); }); } // Get a new owning pointer to an instance of an algorithm. // Throws if the resource isn't available, or if we aren't fully ready yet. template std::unique_ptr get(std::string_view name) const { ensureReady(); - if (!available(name)) { + std::lock_guard lock{m_mutex}; + const auto* factory = find(name); + if (factory == nullptr) { raise(fmt::format("No factory with name {} and type {} registered with the AlgorithmSvc", - name, typeid(AlgoType).name())); + name, detail::demangledName())); } - const auto& factories = m_factories.at(typeid(AlgoType)); - // This creates and object and gives ownership to the unique_ptr - return static_cast(factories.at(name)()); + // This creates an object and gives ownership to the unique_ptr + return std::unique_ptr(static_cast((*factory)().release())); } // Return a vector of the names of all available algorithms of a certain type // Just return an empty vector if no names are present (don't throw any exceptions // for missing entries here). // Throws if we aren't fully ready yet. - template auto ls() const { + template std::vector ls() const { ensureReady(); - std::vector ret; - if (available()) { - std::for_each(factory().begin(), factory().end(), std::back_inserter(ret), - [](auto&& key_value_pair) { return key_value_pair.first(); }); + std::lock_guard lock{m_mutex}; + std::vector ret; + if (const auto it = m_factories.find(typeid(AlgoType)); it != m_factories.end()) { + for (const auto& [name, factory] : it->second) { + ret.push_back(name); + } } return ret; } private: - // Do we have any algorithms available of a certain type? - template bool available() const { return !m_factories.count(typeid(AlgoType)); } - // Do we have any algorithms of type AlgoType with a specific name available? Throws - // if any of the lower-level assumptions fail - template bool available(std::string_view name) const { - if (name == "") { + using factory_type = std::function()>; + + // Find the factory of type AlgoType with a specific name, or nullptr if there is none. + // Throws if the name is invalid + template const factory_type* find(std::string_view name) const { + if (name.empty()) { raise(fmt::format("Invalid name provided: '{}'", name)); } - return factory().count(name) != 0; - } - template const auto& factory() const { - if (!available()) { - raise(fmt::format("No factory for algorithm type {} provided", typeid(AlgoType).name())); + const auto it = m_factories.find(typeid(AlgoType)); + if (it == m_factories.end()) { + return nullptr; } - return m_factories.at(typeid(AlgoType)); + const auto factory = it->second.find(name); + return (factory != it->second.end()) ? &factory->second : nullptr; } // Throw an exception if we are not ready as factory to avoid giving incomplete factory // information to the caller @@ -172,9 +174,10 @@ class AlgorithmSvc : public LoggedService { } } - using factory_type = std::function()>; + mutable std::mutex m_mutex; + std::map>> m_factories; - std::map> m_factories; + ALGORITHMS_DEFINE_LOGGED_SERVICE(AlgorithmSvc) }; namespace detail { diff --git a/core/tests/algorithm_svc.cpp b/core/tests/algorithm_svc.cpp new file mode 100644 index 0000000..ce29bd5 --- /dev/null +++ b/core/tests/algorithm_svc.cpp @@ -0,0 +1,106 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +// Copyright (C) 2026 Wouter Deconinck +// +// Tests for the AlgorithmSvc factory service +// +// The AlgorithmSvc only works after ServiceSvc::init(), which can only be called once per +// process, so the full lifecycle is tested in a single test case. +// +#include + +#include +#include + +#include +#include + +using namespace algorithms; +using Catch::Matchers::ContainsSubstring; + +namespace algorithm_svc_test { + +using IntAlgorithm = Algorithm, Output>; +using DoubleAlgorithm = Algorithm, Output>; + +class AddOne : public IntAlgorithm { +public: + AddOne() : IntAlgorithm{"AddOne", {"in"}, {"out"}, "Adds one"} {} + void process(const Input& input, const Output& output) const final { + *std::get<0>(output) = *std::get<0>(input) + 1; + } +}; + +class Twice : public IntAlgorithm { +public: + Twice() : IntAlgorithm{"Twice", {"in"}, {"out"}, "Multiplies by two"} {} + void process(const Input& input, const Output& output) const final { + *std::get<0>(output) = *std::get<0>(input) * 2; + } +}; + +class Half : public DoubleAlgorithm { +public: + Half() : DoubleAlgorithm{"Half", {"in"}, {"out"}, "Divides by two"} {} + void process(const Input& input, const Output& output) const final { + *std::get<0>(output) = *std::get<0>(input) / 2; + } +}; + +} // namespace algorithm_svc_test + +using namespace algorithm_svc_test; + +TEST_CASE("AlgorithmSvc creates registered algorithms by type and name", "[algorithm]") { + auto& svc = AlgorithmSvc::instance(); + CHECK(svc.name() == "AlgorithmSvc"); + + svc.add(); + svc.add(); + svc.add(); + // adding an algorithm twice has no effect + svc.add(); + + // the service cannot be used before all services are initialized + CHECK_THROWS_AS(svc.ls(), Error); + CHECK_THROWS_WITH(svc.get("algorithm_svc_test::AddOne"), + "Attempt to use AlgorithmSvc, but service not yet marked as ready"); + + ServiceSvc::instance().init(); + REQUIRE(svc.ready()); + + // algorithms are listed by type, with their demangled type name + CHECK(svc.ls() == + std::vector{"algorithm_svc_test::AddOne", "algorithm_svc_test::Twice"}); + CHECK(svc.ls() == std::vector{"algorithm_svc_test::Half"}); + CHECK(svc.ls, Output>>().empty()); + + // each get creates a new instance + const auto add_one = svc.get("algorithm_svc_test::AddOne"); + const auto twice = svc.get("algorithm_svc_test::Twice"); + const auto half = svc.get("algorithm_svc_test::Half"); + REQUIRE(add_one != nullptr); + REQUIRE(twice != nullptr); + REQUIRE(half != nullptr); + CHECK(add_one != svc.get("algorithm_svc_test::AddOne")); + CHECK(add_one->name() == "AddOne"); + CHECK(twice->name() == "Twice"); + + const int i = 3; + int j = 0; + add_one->process({&i}, {&j}); + CHECK(j == 4); + twice->process({&i}, {&j}); + CHECK(j == 6); + + const double x = 3.; + double y = 0.; + half->process({&x}, {&y}); + CHECK(y == 1.5); + + // unknown names, types and invalid names are rejected + CHECK_THROWS_WITH(svc.get("algorithm_svc_test::Half"), + ContainsSubstring("No factory with name algorithm_svc_test::Half and type")); + CHECK_THROWS_WITH((svc.get, Output>>("AddOne")), + ContainsSubstring("No factory with name AddOne")); + CHECK_THROWS_WITH(svc.get(""), "Invalid name provided: ''"); +}