reformat module registry printing and add utest

config_utilities/include/config_utilities/factory.h

@@ -37,6 +37,7 @@
 
 #include <algorithm>
 #include <functional>
+#include <map>
 #include <memory>
 #include <sstream>
 #include <string>
@@ -58,23 +59,35 @@ namespace internal {
 
 class ModuleRegistry {
  public:
-  static void addModule(const std::string& type, const std::string& type_info) {
-    instance().modules.emplace_back(type, type_info);
+  template <typename BaseT, typename DerivedT, typename... Args>
+  static void addModule(const std::string& type) {
+    const std::string base_type = typeName<BaseT>();
+    const std::string derived_type = typeName<DerivedT>();
+    std::stringstream ss;
+    ((ss << typeName<Args>() << ", "), ...);
+    std::string arguments = ss.str();
+    if (!arguments.empty()) {
+      arguments = arguments.substr(0, arguments.size() - 2);
+    }
+
+    auto& types = instance().modules[base_type][arguments];
+    types.emplace_back(type, derived_type);
+    std::sort(types.begin(), types.end());
   }
 
   static std::string getAllRegistered() {
     std::stringstream ss;
     ss << "Modules registered to factories: {";
-    auto modules = instance().modules;
-    std::sort(modules.begin(), modules.end());
-    if (!modules.empty()) {
-      ss << "\n";
-    }
-
-    for (auto&& [type, type_info] : modules) {
-      ss << "  " << type << ": \"" << type_info << "\",\n";
+    for (auto&& [base_type, args] : instance().modules) {
+      for (auto&& [arguments, types] : args) {
+        ss << "\n  " << base_type << "(" << arguments << "): {";
+        for (auto&& [type_name, derived_type] : types) {
+          ss << "\n    '" << type_name << "' (" << derived_type << "),";
+        }
+        ss << "\n  },";
+      }
     }
-    ss << "}";
+    ss << "\n}";
     return ss.str();
   }
 
@@ -86,7 +99,8 @@ class ModuleRegistry {
 
   ModuleRegistry() = default;
 
-  std::vector<std::pair<std::string, std::string>> modules;
+  // Nested modules: base_type -> args -> registered <type_name, tpye>.
+  std::map<std::string, std::map<std::string, std::vector<std::pair<std::string, std::string>>>> modules;
 };
 
 // Struct to store the factory methods for the creation of modules.
@@ -105,7 +119,6 @@ struct ModuleMapBase {
     }
 
     map.insert(std::make_pair(type, method));
-    ModuleRegistry::addModule(type, type_info);
     return true;
   }
 
@@ -249,7 +262,9 @@ struct ObjectFactory {
   template <typename DerivedT>
   static void addEntry(const std::string& type) {
     FactoryMethod method = [](Args... args) { return new DerivedT(args...); };
-    ModuleMap::addEntry(type, method, typeInfo<BaseT, Args...>());
+    if (ModuleMap::addEntry(type, method, typeInfo<BaseT, Args...>())) {
+      ModuleRegistry::addModule<BaseT, DerivedT, Args...>(type);
+    }
   }
 
   static std::unique_ptr<BaseT> create(const std::string& type, Args... args) {
@@ -276,7 +291,9 @@ struct ObjectWithConfigFactory {
       Visitor::setValues(config, data);
       return new DerivedT(config, args...);
     };
-    ModuleMap::addEntry(type, method, typeInfo<BaseT, Args...>());
+    if (ModuleMap::addEntry(type, method, typeInfo<BaseT, Args...>())) {
+      ModuleRegistry::addModule<BaseT, DerivedT, Args...>(type);
+    }
   }
 
   static std::unique_ptr<BaseT> create(const YAML::Node& data, Args... args) {

config_utilities/test/include/config_utilities/test/utils.h

@@ -60,6 +60,8 @@ class TestLogger : public internal::Logger {
   int numMessages() const { return messages_.size(); }
   void clear() { messages_.clear(); }
   void print() const;
+  bool hasMessages() const { return !messages_.empty(); }
+  const std::string& lastMessage() const { return messages_.back().second; }
 
   static std::shared_ptr<TestLogger> create();
 

config_utilities/test/tests/factory.cpp

@@ -102,6 +102,17 @@ void declare_config(DerivedD::Config& config) {
   config::field(config.i, "i");
 }
 
+template <typename T>
+struct TemplatedBase {
+  virtual ~TemplatedBase() = default;
+};
+
+template <typename DerivedT, typename BaseT>
+struct TemplatedDerived : public TemplatedBase<BaseT> {
+  explicit TemplatedDerived(bool b = true) : b_(b) {}
+  const bool b_;
+};
+
 TEST(Factory, create) {
   std::unique_ptr<Base> base = create<Base>("DerivedA", 1);
   EXPECT_TRUE(base);
@@ -164,4 +175,74 @@ TEST(Factory, createWithConfig) {
   EXPECT_EQ(dynamic_cast<DerivedD*>(base.get())->config_.i, 3);
 }
 
+TEST(Factory, moduleNameConflicts) {
+  auto logger = TestLogger::create();
+
+  // Allow shadowing of same name for different module types.
+  const auto registration1 = config::Registration<TemplatedBase<int>, TemplatedDerived<int, int>>("name");
+  const auto registration2 = config::Registration<TemplatedBase<float>, TemplatedDerived<float, float>>("name");
+  EXPECT_EQ(logger->numMessages(), 0);
+
+  // Same derived different name. NOTE(lschmid): This is allowed, not sure if we would want to warn users though.
+  const auto registration3 = config::Registration<TemplatedBase<int>, TemplatedDerived<int, int>>("other_name");
+  EXPECT_FALSE(logger->hasMessages());
+
+  // Same derived same name. Not allowed. NOTE(lschmid): Could also be an option to make this allowed (skip silently).
+  const auto registration4 = config::Registration<TemplatedBase<int>, TemplatedDerived<int, int>>("name");
+  EXPECT_EQ(logger->numMessages(), 1);
+  EXPECT_EQ(logger->lastMessage(),
+            "Cannot register already existent type 'name' for BaseT='config::test::TemplatedBase<int>' and "
+            "ConstructorArguments={}.");
+
+  // Different derived same base and same name. Not allowed.
+  const auto registration5 = config::Registration<TemplatedBase<int>, TemplatedDerived<float, int>>("name");
+  EXPECT_EQ(logger->numMessages(), 2);
+  EXPECT_EQ(logger->lastMessage(),
+            "Cannot register already existent type 'name' for BaseT='config::test::TemplatedBase<int>' and "
+            "ConstructorArguments={}.");
+
+  // Same name, same base but different constructor arguments. Allowed.
+  const auto registration6 = config::Registration<TemplatedBase<int>, TemplatedDerived<int, int>, bool>("name");
+  EXPECT_EQ(logger->numMessages(), 2);
+
+  // Different constructor args with different name. Allowed but not encouraged.
+  const auto registration7 =
+      config::Registration<TemplatedBase<float>, TemplatedDerived<float, float>, bool>("different_name");
+  EXPECT_EQ(logger->numMessages(), 2);
+
+  std::cout << internal::ModuleRegistry::getAllRegistered() << std::endl;
+}
+
+TEST(Factory, printRegistryInfo) {
+  const auto registration1 = config::Registration<TemplatedBase<int>, TemplatedDerived<int, int>>("int_derived");
+  const auto registration2 =
+      config::Registration<TemplatedBase<float>, TemplatedDerived<float, float>>("float_derived");
+  const std::string expected = R"""(Modules registered to factories: {
+  config::internal::Formatter(): {
+    'asl' (config::internal::AslFormatter),
+  },
+  config::test::Base(int): {
+    'DerivedA' (config::test::DerivedA),
+    'DerivedB' (config::test::DerivedB),
+    'DerivedC' (config::test::DerivedC),
+    'DerivedD' (config::test::DerivedD),
+  },
+  config::test::Base2(): {
+    'Derived2' (config::test::Derived2),
+  },
+  config::test::ProcessorBase(): {
+    'AddString' (config::test::AddString),
+  },
+  config::test::TemplatedBase<float>(): {
+    'float_derived' (config::test::TemplatedDerived<float, float>),
+  },
+  config::test::TemplatedBase<int>(): {
+    'int_derived' (config::test::TemplatedDerived<int, int>),
+  },
+})""";
+  const std::string modules = internal::ModuleRegistry::getAllRegistered();
+  std::cout << modules << std::endl;
+  EXPECT_EQ(modules, expected);
+}
+
 }  // namespace config::test