From 4a35323705e4a0ce9b2fbc47504b159009d0d07b Mon Sep 17 00:00:00 2001 From: Andre Nguyen Date: Tue, 25 Feb 2025 17:01:16 -0500 Subject: [PATCH] feat: allow factories to deal with move-only constructor arguments (#44) * feat: allow factories to deal with move-only constructor arguments * fix: Update expected registrations * fix: add missing forwarding reference capture --- .../include/config_utilities/factory.h | 23 ++++--- .../internal/config_context.h | 10 +-- .../config_utilities/parsing/commandline.h | 20 +++--- .../include/config_utilities/parsing/yaml.h | 20 +++--- .../include/config_utilities/virtual_config.h | 5 +- config_utilities/test/tests/factory.cpp | 67 +++++++++++++++++++ 6 files changed, 112 insertions(+), 33 deletions(-) diff --git a/config_utilities/include/config_utilities/factory.h b/config_utilities/include/config_utilities/factory.h index 7c03122..d028372 100644 --- a/config_utilities/include/config_utilities/factory.h +++ b/config_utilities/include/config_utilities/factory.h @@ -242,8 +242,8 @@ class ModuleRegistry { } // wrap factory call to register any allocations - return [factory, key, type, create_callback](Args... args) -> BaseT* { - auto pointer = factory(args...); + return [factory, key, type, create_callback](Args&&... args) -> BaseT* { + auto pointer = factory(std::forward(args)...); create_callback(key, type, pointer); return pointer; }; @@ -367,17 +367,17 @@ struct ObjectFactory { // Add entries. template static void addEntry(const std::string& type) { - const Constructor method = [](Args... args) -> BaseT* { return new DerivedT(args...); }; + const Constructor method = [](Args&&... args) -> BaseT* { return new DerivedT(std::forward(args)...); }; ModuleRegistry::addModule(type, method); } - static std::unique_ptr create(const std::string& type, Args... args) { + static std::unique_ptr create(const std::string& type, Args&&... args) { const auto factory = ModuleRegistry::getModule(type, registration_info); if (!factory) { return nullptr; } - return std::unique_ptr(factory(args...)); + return std::unique_ptr(factory(std::forward(args)...)); } }; @@ -391,16 +391,16 @@ struct ObjectWithConfigFactory { // Add entries. template static void addEntry(const std::string& type) { - const Constructor method = [](const YAML::Node& data, Args... args) -> BaseT* { + const Constructor method = [](const YAML::Node& data, Args&&... args) -> BaseT* { DerivedConfigT config; Visitor::setValues(config, data); - return new DerivedT(config, args...); + return new DerivedT(config, std::forward(args)...); }; ModuleRegistry::addModule(type, method, true); } - static std::unique_ptr create(const YAML::Node& data, Args... args) { + static std::unique_ptr create(const YAML::Node& data, Args&&... args) { std::string type; if (!getType(data, type)) { return nullptr; @@ -411,7 +411,7 @@ struct ObjectWithConfigFactory { return nullptr; } - return std::unique_ptr(factory(data, args...)); + return std::unique_ptr(factory(data, std::forward(args)...)); } }; @@ -468,8 +468,9 @@ struct RegistrationWithConfig { * @returns Unique pointer of type base that contains the derived object. */ template -std::unique_ptr create(const std::string& type, ConstructorArguments... args) { - return internal::ObjectFactory::create(type, args...); +std::unique_ptr create(const std::string& type, ConstructorArguments&&... args) { + return internal::ObjectFactory::create(type, + std::forward(args)...); } } // namespace config diff --git a/config_utilities/include/config_utilities/internal/config_context.h b/config_utilities/include/config_utilities/internal/config_context.h index 7ef5072..087181b 100644 --- a/config_utilities/include/config_utilities/internal/config_context.h +++ b/config_utilities/include/config_utilities/internal/config_context.h @@ -58,14 +58,16 @@ class Context { static YAML::Node toYaml(); template - static std::unique_ptr create(ConstructorArguments... args) { - return internal::ObjectWithConfigFactory::create(instance().contents_, args...); + static std::unique_ptr create(ConstructorArguments&&... args) { + return internal::ObjectWithConfigFactory::create(instance().contents_, + std::forward(args)...); } template - static std::unique_ptr createNamespaced(const std::string& name_space, ConstructorArguments... args) { + static std::unique_ptr createNamespaced(const std::string& name_space, ConstructorArguments&&... args) { const auto ns_node = internal::lookupNamespace(instance().contents_, name_space); - return internal::ObjectWithConfigFactory::create(ns_node, args...); + return internal::ObjectWithConfigFactory::create(ns_node, + std::forward(args)...); } template diff --git a/config_utilities/include/config_utilities/parsing/commandline.h b/config_utilities/include/config_utilities/parsing/commandline.h index 7ed7376..8c6b63a 100644 --- a/config_utilities/include/config_utilities/parsing/commandline.h +++ b/config_utilities/include/config_utilities/parsing/commandline.h @@ -113,10 +113,11 @@ ConfigT fromCLI(const std::vector& args, const std::string& name_sp * @returns Unique pointer of type base that contains the derived object. */ template -std::unique_ptr createFromCLI(int argc, char* argv[], ConstructorArguments... args) { +std::unique_ptr createFromCLI(int argc, char* argv[], ConstructorArguments&&... args) { // when parsing CLI locally we don't want to modify the arguments ever const auto node = internal::loadFromArguments(argc, argv, false); - return internal::ObjectWithConfigFactory::create(node, args...); + return internal::ObjectWithConfigFactory::create(node, + std::forward(args)...); } /** @@ -131,9 +132,10 @@ std::unique_ptr createFromCLI(int argc, char* argv[], ConstructorArgument * @returns Unique pointer of type base that contains the derived object. */ template -std::unique_ptr createFromCLI(const std::vector& argv, ConstructorArguments... args) { +std::unique_ptr createFromCLI(const std::vector& argv, ConstructorArguments&&... args) { const auto node = internal::loadFromArguments(argv); - return internal::ObjectWithConfigFactory::create(node, args...); + return internal::ObjectWithConfigFactory::create(node, + std::forward(args)...); } /** @@ -153,11 +155,12 @@ template std::unique_ptr createFromCLIWithNamespace(int argc, char* argv[], const std::string& name_space, - ConstructorArguments... args) { + ConstructorArguments&&... args) { // when parsing CLI locally we don't want to modify the arguments ever const auto node = internal::loadFromArguments(argc, argv, false); const auto ns_node = internal::lookupNamespace(node, name_space); - return internal::ObjectWithConfigFactory::create(ns_node, args...); + return internal::ObjectWithConfigFactory::create(ns_node, + std::forward(args)...); } /** @@ -175,10 +178,11 @@ std::unique_ptr createFromCLIWithNamespace(int argc, template std::unique_ptr createFromCLIWithNamespace(const std::vector& argv, const std::string& name_space, - ConstructorArguments... args) { + ConstructorArguments&&... args) { const auto node = internal::loadFromArguments(argv); const auto ns_node = internal::lookupNamespace(node, name_space); - return internal::ObjectWithConfigFactory::create(ns_node, args...); + return internal::ObjectWithConfigFactory::create(ns_node, + std::forward(args)...); } } // namespace config diff --git a/config_utilities/include/config_utilities/parsing/yaml.h b/config_utilities/include/config_utilities/parsing/yaml.h index 608f2da..1251504 100644 --- a/config_utilities/include/config_utilities/parsing/yaml.h +++ b/config_utilities/include/config_utilities/parsing/yaml.h @@ -129,8 +129,9 @@ bool toYamlFile(const ConfigT& config, const std::string& file_name) { * @returns Unique pointer of type base that contains the derived object. */ template -std::unique_ptr createFromYaml(const YAML::Node& node, ConstructorArguments... args) { - return internal::ObjectWithConfigFactory::create(node, args...); +std::unique_ptr createFromYaml(const YAML::Node& node, ConstructorArguments&&... args) { + return internal::ObjectWithConfigFactory::create(node, + std::forward(args)...); } /** @@ -151,9 +152,10 @@ std::unique_ptr createFromYaml(const YAML::Node& node, ConstructorArgumen template std::unique_ptr createFromYamlWithNamespace(const YAML::Node& node, const std::string& name_space, - ConstructorArguments... args) { + ConstructorArguments&&... args) { const YAML::Node ns_node = internal::lookupNamespace(node, name_space); - return internal::ObjectWithConfigFactory::create(ns_node, args...); + return internal::ObjectWithConfigFactory::create(ns_node, + std::forward(args)...); } /** @@ -170,8 +172,9 @@ std::unique_ptr createFromYamlWithNamespace(const YAML::Node& node, * @returns Unique pointer of type base that contains the derived object. */ template -std::unique_ptr createFromYamlFile(const std::string& file_name, ConstructorArguments... args) { - return internal::ObjectWithConfigFactory::create(YAML::LoadFile(file_name), args...); +std::unique_ptr createFromYamlFile(const std::string& file_name, ConstructorArguments&&... args) { + return internal::ObjectWithConfigFactory::create(YAML::LoadFile(file_name), + std::forward(args)...); } /** @@ -192,9 +195,10 @@ std::unique_ptr createFromYamlFile(const std::string& file_name, Construc template std::unique_ptr createFromYamlFileWithNamespace(const std::string& file_name, const std::string& name_space, - ConstructorArguments... args) { + ConstructorArguments&&... args) { const YAML::Node node = internal::lookupNamespace(YAML::LoadFile(file_name), name_space); - return internal::ObjectWithConfigFactory::create(node, args...); + return internal::ObjectWithConfigFactory::create(node, + std::forward(args)...); } /** diff --git a/config_utilities/include/config_utilities/virtual_config.h b/config_utilities/include/config_utilities/virtual_config.h index f3db149..32ed7a1 100644 --- a/config_utilities/include/config_utilities/virtual_config.h +++ b/config_utilities/include/config_utilities/virtual_config.h @@ -172,7 +172,7 @@ class VirtualConfig { * @return The created object of DerivedT that inherits from BaseT. */ template - std::unique_ptr create(ConstructorArguments... args) const { + std::unique_ptr create(ConstructorArguments&&... args) const { if (!config_) { return nullptr; } @@ -181,7 +181,8 @@ class VirtualConfig { // also be de-serialized so this should not result in any warnings, we print them anyways to be sure. The factory // should take proper care of any other verbose error management. const internal::MetaData data = internal::Visitor::getValues(*this); - return internal::ObjectWithConfigFactory::create(data.data, args...); + return internal::ObjectWithConfigFactory::create(data.data, + std::forward(args)...); } private: diff --git a/config_utilities/test/tests/factory.cpp b/config_utilities/test/tests/factory.cpp index eab5ae7..23707bd 100644 --- a/config_utilities/test/tests/factory.cpp +++ b/config_utilities/test/tests/factory.cpp @@ -194,6 +194,64 @@ TEST(Factory, createWithConfig) { EXPECT_EQ(dynamic_cast(base.get())->config_.i, 3); } +struct MoveOnlyBase { + virtual ~MoveOnlyBase() = default; + virtual void print() { std::cout << "Hi! I'm Base\n"; } +}; + +struct MoveOnlyDerived : public MoveOnlyBase { + explicit MoveOnlyDerived(std::unique_ptr i) : i_(std::move(i)) {} + ~MoveOnlyDerived() override = default; + + std::unique_ptr i_; + inline static const auto registration = + config::Registration>("MoveOnlyDerived"); +}; + +struct MoveOnlyDerivedWithConfig : public MoveOnlyBase { + struct Config { + int i = 0; + }; + + explicit MoveOnlyDerivedWithConfig(const Config& config, std::unique_ptr i) : config_(config), i_(std::move(i)) {} + ~MoveOnlyDerivedWithConfig() override = default; + + Config config_; + std::unique_ptr i_; + inline static const auto registration = + config::RegistrationWithConfig>( + "MoveOnlyDerivedWithConfig"); +}; + +void declare_config(MoveOnlyDerivedWithConfig::Config& config) { + // Declare the config using the config utilities. + config::name("MoveOnlyDerivedWithConfig"); + config::field(config.i, "i"); +} + +TEST(Factory, createWithMoveOnlyArgument) { + { + auto i = std::make_unique(42); + auto base = config::create("MoveOnlyDerived", std::move(i)); + EXPECT_NE(base, nullptr); + auto ptr = dynamic_cast(base.get()); + EXPECT_NE(ptr, nullptr); + EXPECT_EQ(*ptr->i_, 42); + } + { + auto i = std::make_unique(24); + YAML::Node yaml; + yaml["type"] = "MoveOnlyDerivedWithConfig"; + yaml["i"] = 24; + + auto base = config::createFromYaml(yaml, std::move(i)); + EXPECT_NE(base, nullptr); + auto ptr = dynamic_cast(base.get()); + EXPECT_NE(ptr, nullptr); + EXPECT_EQ(*ptr->i_, 24); + } +} + TEST(Factory, moduleNameConflicts) { auto logger = TestLogger::create(); @@ -250,6 +308,9 @@ config::test::Base(int): 'DerivedA' (config::test::DerivedA) 'DerivedB' (config::test::DerivedB) +config::test::MoveOnlyBase(std::unique_ptr >): + 'MoveOnlyDerived' (config::test::MoveOnlyDerived) + config::test::Talker(): 'internal' (config::test::InternalTalker) @@ -280,6 +341,9 @@ config::test::Base2(): 'Derived2' (config::test::Derived2) 'Derived2A' (config::test::Derived2A) +config::test::MoveOnlyBase(std::unique_ptr >): + 'MoveOnlyDerivedWithConfig' (config::test::MoveOnlyDerivedWithConfig) + config::test::ProcessorBase(): 'AddString' (config::test::AddString) @@ -298,6 +362,9 @@ Config[config::test::Base2](): 'Derived2' (config::test::Derived2::Config) 'Derived2A' (config::test::Derived2A::Config) +Config[config::test::MoveOnlyBase](): + 'MoveOnlyDerivedWithConfig' (config::test::MoveOnlyDerivedWithConfig::Config) + Config[config::test::ProcessorBase](): 'AddString' (config::test::AddString::Config)