Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow factories to deal with move-only constructor arguments #44

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions config_utilities/include/config_utilities/factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>(args)...);
create_callback(key, type, pointer);
return pointer;
};
Expand Down Expand Up @@ -367,17 +367,17 @@ struct ObjectFactory {
// Add entries.
template <typename DerivedT>
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>(args)...); };
ModuleRegistry::addModule<BaseT, DerivedT, Args...>(type, method);
}

static std::unique_ptr<BaseT> create(const std::string& type, Args... args) {
static std::unique_ptr<BaseT> create(const std::string& type, Args&&... args) {
const auto factory = ModuleRegistry::getModule<BaseT, Args...>(type, registration_info);
if (!factory) {
return nullptr;
}

return std::unique_ptr<BaseT>(factory(args...));
return std::unique_ptr<BaseT>(factory(std::forward<Args>(args)...));
}
};

Expand All @@ -391,16 +391,16 @@ struct ObjectWithConfigFactory {
// Add entries.
template <typename DerivedT, typename DerivedConfigT>
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>(args)...);
};

ModuleRegistry::addModule<BaseT, DerivedT, const YAML::Node&, Args...>(type, method, true);
}

static std::unique_ptr<BaseT> create(const YAML::Node& data, Args... args) {
static std::unique_ptr<BaseT> create(const YAML::Node& data, Args&&... args) {
std::string type;
if (!getType(data, type)) {
return nullptr;
Expand All @@ -411,7 +411,7 @@ struct ObjectWithConfigFactory {
return nullptr;
}

return std::unique_ptr<BaseT>(factory(data, args...));
return std::unique_ptr<BaseT>(factory(data, std::forward<Args>(args)...));
}
};

Expand Down Expand Up @@ -468,8 +468,9 @@ struct RegistrationWithConfig {
* @returns Unique pointer of type base that contains the derived object.
*/
template <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> create(const std::string& type, ConstructorArguments... args) {
return internal::ObjectFactory<BaseT, ConstructorArguments...>::create(type, args...);
std::unique_ptr<BaseT> create(const std::string& type, ConstructorArguments&&... args) {
return internal::ObjectFactory<BaseT, ConstructorArguments...>::create(type,
std::forward<ConstructorArguments>(args)...);
}

} // namespace config
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,16 @@ class Context {
static YAML::Node toYaml();

template <typename BaseT, typename... ConstructorArguments>
static std::unique_ptr<BaseT> create(ConstructorArguments... args) {
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(instance().contents_, args...);
static std::unique_ptr<BaseT> create(ConstructorArguments&&... args) {
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(instance().contents_,
std::forward<ConstructorArguments>(args)...);
}

template <typename BaseT, typename... ConstructorArguments>
static std::unique_ptr<BaseT> createNamespaced(const std::string& name_space, ConstructorArguments... args) {
static std::unique_ptr<BaseT> createNamespaced(const std::string& name_space, ConstructorArguments&&... args) {
const auto ns_node = internal::lookupNamespace(instance().contents_, name_space);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(ns_node, args...);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(ns_node,
std::forward<ConstructorArguments>(args)...);
}

template <typename ConfigT>
Expand Down
20 changes: 12 additions & 8 deletions config_utilities/include/config_utilities/parsing/commandline.h
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,11 @@ ConfigT fromCLI(const std::vector<std::string>& args, const std::string& name_sp
* @returns Unique pointer of type base that contains the derived object.
*/
template <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> createFromCLI(int argc, char* argv[], ConstructorArguments... args) {
std::unique_ptr<BaseT> 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<BaseT, ConstructorArguments...>::create(node, args...);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(node,
std::forward<ConstructorArguments>(args)...);
}

/**
Expand All @@ -131,9 +132,10 @@ std::unique_ptr<BaseT> createFromCLI(int argc, char* argv[], ConstructorArgument
* @returns Unique pointer of type base that contains the derived object.
*/
template <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> createFromCLI(const std::vector<std::string>& argv, ConstructorArguments... args) {
std::unique_ptr<BaseT> createFromCLI(const std::vector<std::string>& argv, ConstructorArguments&&... args) {
const auto node = internal::loadFromArguments(argv);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(node, args...);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(node,
std::forward<ConstructorArguments>(args)...);
}

/**
Expand All @@ -153,11 +155,12 @@ template <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> 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<BaseT, ConstructorArguments...>::create(ns_node, args...);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(ns_node,
std::forward<ConstructorArguments>(args)...);
}

/**
Expand All @@ -175,10 +178,11 @@ std::unique_ptr<BaseT> createFromCLIWithNamespace(int argc,
template <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> createFromCLIWithNamespace(const std::vector<std::string>& 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<BaseT, ConstructorArguments...>::create(ns_node, args...);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(ns_node,
std::forward<ConstructorArguments>(args)...);
}

} // namespace config
20 changes: 12 additions & 8 deletions config_utilities/include/config_utilities/parsing/yaml.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> createFromYaml(const YAML::Node& node, ConstructorArguments... args) {
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(node, args...);
std::unique_ptr<BaseT> createFromYaml(const YAML::Node& node, ConstructorArguments&&... args) {
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(node,
std::forward<ConstructorArguments>(args)...);
}

/**
Expand All @@ -151,9 +152,10 @@ std::unique_ptr<BaseT> createFromYaml(const YAML::Node& node, ConstructorArgumen
template <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> 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<BaseT, ConstructorArguments...>::create(ns_node, args...);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(ns_node,
std::forward<ConstructorArguments>(args)...);
}

/**
Expand All @@ -170,8 +172,9 @@ std::unique_ptr<BaseT> createFromYamlWithNamespace(const YAML::Node& node,
* @returns Unique pointer of type base that contains the derived object.
*/
template <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> createFromYamlFile(const std::string& file_name, ConstructorArguments... args) {
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(YAML::LoadFile(file_name), args...);
std::unique_ptr<BaseT> createFromYamlFile(const std::string& file_name, ConstructorArguments&&... args) {
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(YAML::LoadFile(file_name),
std::forward<ConstructorArguments>(args)...);
}

/**
Expand All @@ -192,9 +195,10 @@ std::unique_ptr<BaseT> createFromYamlFile(const std::string& file_name, Construc
template <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> 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<BaseT, ConstructorArguments...>::create(node, args...);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(node,
std::forward<ConstructorArguments>(args)...);
}

/**
Expand Down
5 changes: 3 additions & 2 deletions config_utilities/include/config_utilities/virtual_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class VirtualConfig {
* @return The created object of DerivedT that inherits from BaseT.
*/
template <typename... ConstructorArguments>
std::unique_ptr<BaseT> create(ConstructorArguments... args) const {
std::unique_ptr<BaseT> create(ConstructorArguments&&... args) const {
if (!config_) {
return nullptr;
}
Expand All @@ -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<BaseT, ConstructorArguments...>::create(data.data, args...);
return internal::ObjectWithConfigFactory<BaseT, ConstructorArguments...>::create(data.data,
std::forward<ConstructorArguments>(args)...);
}

private:
Expand Down
67 changes: 67 additions & 0 deletions config_utilities/test/tests/factory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,64 @@ TEST(Factory, createWithConfig) {
EXPECT_EQ(dynamic_cast<DerivedD*>(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<int> i) : i_(std::move(i)) {}
~MoveOnlyDerived() override = default;

std::unique_ptr<int> i_;
inline static const auto registration =
config::Registration<MoveOnlyBase, MoveOnlyDerived, std::unique_ptr<int>>("MoveOnlyDerived");
};

struct MoveOnlyDerivedWithConfig : public MoveOnlyBase {
struct Config {
int i = 0;
};

explicit MoveOnlyDerivedWithConfig(const Config& config, std::unique_ptr<int> i) : config_(config), i_(std::move(i)) {}
~MoveOnlyDerivedWithConfig() override = default;

Config config_;
std::unique_ptr<int> i_;
inline static const auto registration =
config::RegistrationWithConfig<MoveOnlyBase, MoveOnlyDerivedWithConfig, Config, std::unique_ptr<int>>(
"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<int>(42);
auto base = config::create<MoveOnlyBase>("MoveOnlyDerived", std::move(i));
EXPECT_NE(base, nullptr);
auto ptr = dynamic_cast<MoveOnlyDerived*>(base.get());
EXPECT_NE(ptr, nullptr);
EXPECT_EQ(*ptr->i_, 42);
}
{
auto i = std::make_unique<int>(24);
YAML::Node yaml;
yaml["type"] = "MoveOnlyDerivedWithConfig";
yaml["i"] = 24;

auto base = config::createFromYaml<MoveOnlyBase>(yaml, std::move(i));
EXPECT_NE(base, nullptr);
auto ptr = dynamic_cast<MoveOnlyDerivedWithConfig*>(base.get());
EXPECT_NE(ptr, nullptr);
EXPECT_EQ(*ptr->i_, 24);
}
}

TEST(Factory, moduleNameConflicts) {
auto logger = TestLogger::create();

Expand Down Expand Up @@ -250,6 +308,9 @@ config::test::Base(int):
'DerivedA' (config::test::DerivedA)
'DerivedB' (config::test::DerivedB)
config::test::MoveOnlyBase(std::unique_ptr<int, std::default_delete<int> >):
'MoveOnlyDerived' (config::test::MoveOnlyDerived)
config::test::Talker():
'internal' (config::test::InternalTalker)
Expand Down Expand Up @@ -280,6 +341,9 @@ config::test::Base2():
'Derived2' (config::test::Derived2)
'Derived2A' (config::test::Derived2A)
config::test::MoveOnlyBase(std::unique_ptr<int, std::default_delete<int> >):
'MoveOnlyDerivedWithConfig' (config::test::MoveOnlyDerivedWithConfig)
config::test::ProcessorBase():
'AddString' (config::test::AddString)
Expand All @@ -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)
Expand Down