C++ Class for optionally constructing arbitrary derived classes

Question

Here is a class I'm working up to register classes derived from a Module base class. These classes may or may not be instantiated (via the ModuleCreatorBase::create() method) based on some logic which is not yet included in the code.

I wanted to allow constructing Modules which take no arguments and Modules which take any number of arguments, though I'm not sure if my method here is the best approach for that. For some reason it feels kind of clunky to me.

Are there more effective or perhaps other more well established ways to achieve this?

#include <memory>
#include <unordered_map>
#include "Module.hpp"
class ModuleManager
{
public:
 template <typename ModuleClass>
 static void registerModule(const std::string& moduleName);
 template <typename ModuleClass, typename... Args>
 static void registerModule(const std::string& moduleName, Args&&... args);
private:
 struct ModuleCreatorBase
 {
 virtual std::unique_ptr<Module> create() = 0;
 };
 template <typename ModuleClass>
 struct ModuleCreator : public ModuleCreatorBase
 {
 std::unique_ptr<Module> create() override;
 };
 template <typename ModuleClass, typename... Args>
 struct ModuleCreatorArgs : public ModuleCreatorBase
 {
 ModuleCreatorArgs(Args&&... args);
 std::tuple<Args...> args;
 template<std::size_t ...I>
 std::unique_ptr<Module> do_create(std::index_sequence<I...>);
 std::unique_ptr<Module> create() override;
 };
 static std::unordered_map<std::string, ModuleCreatorBase*> registry;
};
template <typename ModuleClass>
std::unique_ptr<Module> ModuleManager::ModuleCreator<ModuleClass>::create()
{
 return std::make_unique<ModuleClass>();
}
template <typename ModuleClass, typename... Args>
ModuleManager::ModuleCreatorArgs<ModuleClass, Args...>::ModuleCreatorArgs(Args&&... args)
: args(std::forward<Args>(args)...) {}
template <typename ModuleClass, typename... Args>
std::unique_ptr<Module> ModuleManager::ModuleCreatorArgs<ModuleClass, Args...>::create()
{
 return do_create(std::index_sequence_for<Args...>{});
}
template <typename ModuleClass, typename... Args>
template <std::size_t ...I>
std::unique_ptr<Module> ModuleManager::ModuleCreatorArgs<ModuleClass, Args...>::do_create(std::index_sequence<I...>)
{
 return std::make_unique<ModuleClass>(std::get<I>(args)...);
}
template <typename ModuleClass>
void ModuleManager::registerModule(const std::string& moduleName)
{
 registry.insert(std::pair<std::string, ModuleCreatorBase*>(moduleName, new ModuleCreator<ModuleClass>()));
}
template <typename ModuleClass, typename... Args>
void ModuleManager::registerModule(const std::string& moduleName, Args&&... args)
{
 registry.insert(std::pair<std::string, ModuleCreatorBase*>(moduleName, new ModuleCreatorArgs<ModuleClass, Args...>(std::forward<Args>(args)...)));
}

• hoffmale: Currently, Modules constructed with parameters get a std::tuple containing those. Is this intended?
• Phrancis: Welcome to Code Review! I hope you get some great answers.
• NanoWizard: @hoffmale code has been updated to unpack the constructor parameters. Thank you for pointing that out :)

Answer 1

Unconstrained templates

The public facing member function templates registerModule never validate any of the types they take. Currently, trying to call registerModule for a type not derived from Module fails due to the instantiation of ModuleCreator::create (and subsequent missing conversion from ModuleClass to Module), this relies on an implementation detail (which might change).

Similarly, the overload of registerModule which takes constructor arguments never validates whether ModuleClass can actually be constructed with them.

Better be explicit about which types are allowed to be passed!

In C++17, one could use a simple if constexpr, but since this question is tagged c++14, using SFINAE is the only option.

Even better, with slightly more work you can give much better error messages than what the compiler generates, so a user of the class doesn't have to parse dozens of lines of template instantiations.

Example using SFINAE (just member function declaration), assuming below fix for ModuleCreatorArgs::args:

template<typename ModuleClass>
static std::enable_if_t<
 std::is_base_of<Module, ModuleClass>::value, void> &&
 std::is_default_constructible<ModuleClass>::value,
void> registerModule(const std::string&);
template<typename ModuleClass, typename... Args>
static std::enable_if_t<
 std::is_base_of<Module, ModuleClass>::value &&
 std::is_constructible<ModuleClass, std::remove_reference_t<Args>...>::value,
void> registerModule(const std::string&, Args&&...);

For error messages, you could add:

template<typename ModuleClass, typename...Args>
static std::enable_if_t<
 !std::is_base_of<Module, ModuleClass>::value ||
 !std::is_constructible<ModuleClass, std::remove_reference_t<Args>...>::value,
void> registerModule(const std::string&, Args&&...)
{
 static_assert(std::is_base_of<Module, ModuleClass>::value, "ModuleClass needs to be derived from Module!");
 static_assert(std::is_constructible<ModuleClass, std::remove_reference_t<Args>...>::value, "ModuleClass is not constructible from the given arguments!");
}

Memory and lifetime management

ModuleManager leaks all created instance of ModuleCreatorBase stored in ModuleManager::registry, as delete is never called. Why not simple use std::unique_ptr<ModuleCreatorBase> instead of ModuleCreatorBase*? (Note: adding calls to delete in the destructor of ModuleManager doesn't fix this problem!)

A bit subtler is the dangling reference issue with ModuleCreatorArgs: Since all the passed arguments are perfectly forwarded to be stored in ModuleCreatorArgs::args, lvalue references (to possibly local values!) will be stored as references, which might dangle when they are actually used for Module construction.

Now, I don't know the requirements for Modules derived classes: Do they need to take references in their constructors (e.g. for non-copyable non-movable objects) that for some reason cannot be passed as pointers?

• If no, this gets simple: Change the type of ModuleCreatorArgs::args to std::tuple<std::remove_reference_t<Args>...> so every argument is either moved or copied (as appropriate).
• If yes, this will require good documentation and discipline! (Honestly, if you can, try to avoid this mess. This is easy to screw up and hard to get right at the callsite to ModuleManager::registerModule.)

Final notes

I really hopes this class (ModuleManager) gets expanded so it can actually be used to construct objects (all it currently does is creating private state). Maybe add a ModuleManager::create(const std::string&) method?

Also, I don't know your actual use case of Modules, but if you expect them to be shared quite often, you might want to add a create_shared (or similarly named) method that returns a std::shared_ptr<Module> instead.

• NanoWizard: These are excellent points! Could I not use a static_assert to provide better error messages for template instantiation? Could you provide an example of how to use that or SFINAE effectively? (Yes I'd like to constrain the project to c++14). I will certainly be taking your memory management recommendations into account. As to your final point, I want this class to be focused solely on instantiating Modules. What happens to those Modules later (whether they get shared, etc.) happens somewhere else by design.
• hoffmale: @NanoWizard: Added a SFINAE example to the answer.
• Incomputable: @NanoWizard, to address memory problem, you could write something that behaves like std::thread's constructor: copies/moves always. Make correct usage easy to use, and more subtler ones harder. std::thread uses std::ref to store references, if one desires.