Simplify registry code (paceholder#171)

Author: Quincunx271

.travis.yml

@@ -23,7 +23,12 @@ matrix:
       dist: xenial
       sudo: false
       compiler: gcc
-      env: CXX=g++-7 CC=gcc-7 QT=512
+      env:
+        - CXX=g++-7 CC=gcc-7 QT=512
+        - CXXFLAGS="-fsanitize=address -fno-omit-frame-pointer"
+        - LDFLAGS=-fsanitize=address
+           # Too many false positive leaks:
+        - ASAN_OPTIONS=detect_leaks=0
       addons:
         apt:
           sources:

include/nodes/internal/DataModelRegistry.hpp

@@ -1,9 +1,11 @@
 #pragma once
 
-#include <set>
-#include <memory>
 #include <functional>
+#include <memory>
+#include <set>
+#include <type_traits>
 #include <unordered_map>
+#include <utility>
 #include <vector>
 
 #include <QtCore/QString>
@@ -55,21 +57,40 @@ class NODE_EDITOR_PUBLIC DataModelRegistry
   void registerModel(RegistryItemCreator creator,
                      QString const &category = "Nodes")
   {
-    registerModelImpl<ModelType>(std::move(creator), category);
+    const QString name = computeName<ModelType>(HasStaticMethodName<ModelType>{}, creator);
+    if (!_registeredItemCreators.count(name))
+    {
+      _registeredItemCreators[name] = std::move(creator);
+      _categories.insert(category);
+      _registeredModelsCategory[name] = category;
+    }
   }
 
   template<typename ModelType>
   void registerModel(QString const &category = "Nodes")
   {
     RegistryItemCreator creator = [](){ return std::make_unique<ModelType>(); };
-    registerModelImpl<ModelType>(std::move(creator), category);
+    registerModel<ModelType>(std::move(creator), category);
   }
 
   template<typename ModelType>
   void registerModel(QString const &category,
                      RegistryItemCreator creator)
   {
-    registerModelImpl<ModelType>(std::move(creator), category);
+    registerModel<ModelType>(std::move(creator), category);
+  }
+
+  template <typename ModelCreator>
+  void registerModel(ModelCreator&& creator, QString const& category = "Nodes")
+  {
+    using ModelType = compute_model_type_t<decltype(creator())>;
+    registerModel<ModelType>(std::forward<ModelCreator>(creator), category);
+  }
+
+  template <typename ModelCreator>
+  void registerModel(QString const& category, ModelCreator&& creator)
+  {
+    registerModel(std::forward<ModelCreator>(creator), category);
   }
 
   void registerTypeConverter(TypeConverterId const & id,
@@ -120,32 +141,40 @@ class NODE_EDITOR_PUBLIC DataModelRegistry
       : std::true_type
   {};
 
-  template<typename ModelType>
-  typename std::enable_if< HasStaticMethodName<ModelType>::value>::type
-  registerModelImpl(RegistryItemCreator creator, QString const &category )
+  template <typename ModelType>
+  static QString
+  computeName(std::true_type, RegistryItemCreator const&)
   {
-    const QString name = ModelType::Name();
-    if (_registeredItemCreators.count(name) == 0)
-    {
-      _registeredItemCreators[name] = std::move(creator);
-      _categories.insert(category);
-      _registeredModelsCategory[name] = category;
-    }
+    return ModelType::Name();
   }
 
-  template<typename ModelType>
-  typename std::enable_if< !HasStaticMethodName<ModelType>::value>::type
-  registerModelImpl(RegistryItemCreator creator, QString const &category )
+  template <typename ModelType>
+  static QString
+  computeName(std::false_type, RegistryItemCreator const& creator)
   {
-    const QString name = creator()->name();
-    if (_registeredItemCreators.count(name) == 0)
-    {
-      _registeredItemCreators[name] = std::move(creator);
-      _categories.insert(category);
-      _registeredModelsCategory[name] = category;
-    }
+    return creator()->name();
   }
 
+  template <typename T>
+  struct UnwrapUniquePtr
+  {
+    // Assert always fires, but the compiler doesn't know this:
+    static_assert(!std::is_same<T, T>::value,
+                  "The ModelCreator must return a std::unique_ptr<T>, where T "
+                  "inherits from NodeDataModel");
+  };
+
+  template <typename T>
+  struct UnwrapUniquePtr<std::unique_ptr<T>>
+  {
+    static_assert(std::is_base_of<NodeDataModel, T>::value,
+                  "The ModelCreator must return a std::unique_ptr<T>, where T "
+                  "inherits from NodeDataModel");
+    using type = T;
+  };
+
+  template <typename CreatorResult>
+  using compute_model_type_t = typename UnwrapUniquePtr<CreatorResult>::type;
 };
 
 

include/nodes/internal/FlowScene.hpp

@@ -139,9 +139,14 @@ class NODE_EDITOR_PUBLIC FlowScene
   using SharedConnection = std::shared_ptr<Connection>;
   using UniqueNode       = std::unique_ptr<Node>;
 
+  // DO NOT reorder this member to go after the others.
+  // This should outlive all the connections and nodes of
+  // the graph, so that nodes can potentially have pointers into it,
+  // which is why it comes first in the class.
+  std::shared_ptr<DataModelRegistry> _registry;
+
   std::unordered_map<QUuid, SharedConnection> _connections;
   std::unordered_map<QUuid, UniqueNode>       _nodes;
-  std::shared_ptr<DataModelRegistry>          _registry;
 
 private Q_SLOTS:
 

src/Connection.cpp

@@ -66,7 +66,11 @@ Connection(Node& nodeIn,
 Connection::
 ~Connection()
 {
-  if (complete()) connectionMadeIncomplete(*this);
+  if (complete())
+  {
+    connectionMadeIncomplete(*this);
+  }
+
   propagateEmptyData();
 
   if (_inNode)
@@ -356,7 +360,8 @@ void
 Connection::
 clearNode(PortType portType)
 {
-  if (complete()) {
+  if (complete())
+  {
     connectionMadeIncomplete(*this);
   }
 

src/FlowScene.cpp

@@ -185,7 +185,8 @@ FlowScene::
 deleteConnection(Connection& connection)
 {
   auto it = _connections.find(connection.id());
-  if (it != _connections.end()) {
+  if (it != _connections.end())
+  {
     connection.removeFromNodes();
     _connections.erase(it);
   }

test/CMakeLists.txt

@@ -5,8 +5,8 @@ add_executable(test_nodes
   test_main.cpp
   src/TestDragging.cpp
   src/TestDataModelRegistry.cpp
-  src/TestNodeGraphicsObject.cpp
   src/TestFlowScene.cpp
+  src/TestNodeGraphicsObject.cpp
 )
 
 target_include_directories(test_nodes

test/src/TestDataModelRegistry.cpp

@@ -42,4 +42,31 @@ TEST_CASE("DataModelRegistry::registerModel", "[interface]")
 
     CHECK(model->name() == "name");
   }
+  SECTION("From model creator function")
+  {
+    SECTION("non-static name()")
+    {
+      registry.registerModel([] {
+        return std::make_unique<StubNodeDataModel>();
+      });
+
+      auto model = registry.create("name");
+
+      REQUIRE(model != nullptr);
+      CHECK(model->name() == "name");
+      CHECK(dynamic_cast<StubNodeDataModel*>(model.get()));
+    }
+    SECTION("static Name()")
+    {
+      registry.registerModel([] {
+        return std::make_unique<StubModelStaticName>();
+      });
+
+      auto model = registry.create("Name");
+
+      REQUIRE(model != nullptr);
+      CHECK(model->name() == "name");
+      CHECK(dynamic_cast<StubModelStaticName*>(model.get()));
+    }
+  }
 }

test/src/TestFlowScene.cpp

@@ -125,15 +125,19 @@ TEST_CASE("FlowScene triggers connections created or deleted", "[gui]")
   struct Deletion
   {
     std::string                                      name;
-    std::function<void(std::shared_ptr<Connection>)> deleteConnection;
+    std::function<void(Connection & connection)> deleteConnection;
   };
 
   Deletion sceneDeletion{"scene.deleteConnection",
-                         [&](auto c) { scene.deleteConnection(*c); }};
+                         [&](Connection & c) { scene.deleteConnection(c); }};
 
   Deletion partialDragDeletion{"scene-deleteConnectionByDraggingOff",
-                               [&](auto connection) {
-                                 connection->clearNode(PortType::In);
+                               [&](Connection & c)
+                               {
+                                 PortIndex portIndex = c.getPortIndex(PortType::In);
+                                 Node * node = c.getNode(PortType::In);
+                                 node->nodeState().getEntries(PortType::In)[portIndex].clear();
+                                 c.clearNode(PortType::In);
                                }};
 
   SECTION("creating a connection")
@@ -168,13 +172,14 @@ TEST_CASE("FlowScene triggers connections created or deleted", "[gui]")
     {
       SECTION("deletion: " + deletion.name)
       {
-        auto                      connection     = sceneCreation.createConnection();
-        std::weak_ptr<Connection> weakConnection = connection;
+        Connection & connection = *sceneCreation.createConnection();
 
         from.resetCallCounts();
         to.resetCallCounts();
 
-        deletion.deleteConnection(std::move(connection));
+        deletion.deleteConnection(connection);
+
+        // Here the Connection reference becomes dangling
 
         CHECK(from.inputDeletedCalledCount == 0);
         CHECK(from.outputDeletedCalledCount == 1);
@@ -188,3 +193,64 @@ TEST_CASE("FlowScene triggers connections created or deleted", "[gui]")
     }
   }
 }
+
+
+TEST_CASE("FlowScene's DataModelRegistry outlives nodes and connections", "[asan][gui]")
+{
+  class MockDataModel : public StubNodeDataModel
+  {
+  public:
+    MockDataModel(int* const& incrementOnDestruction)
+      : incrementOnDestruction(incrementOnDestruction)
+    {
+    }
+
+    ~MockDataModel()
+    {
+      (*incrementOnDestruction)++;
+    }
+
+    // The reference ensures that we point into the memory that would be free'd
+    // if the DataModelRegistry doesn't outlive this node
+    int* const& incrementOnDestruction;
+  };
+
+  struct MockDataModelCreator
+  {
+    MockDataModelCreator(int* shouldBeAliveWhenAssignedTo)
+      : shouldBeAliveWhenAssignedTo(shouldBeAliveWhenAssignedTo)
+    {
+    }
+
+    auto
+    operator()() const
+    {
+      return std::make_unique<MockDataModel>(shouldBeAliveWhenAssignedTo);
+    }
+
+    int* shouldBeAliveWhenAssignedTo;
+  };
+
+  int modelsDestroyed = 0;
+
+  // Introduce a new scope, so that modelsDestroyed will be alive even after the
+  // FlowScene is destroyed.
+  {
+    auto setup = applicationSetup();
+
+    auto registry = std::make_shared<DataModelRegistry>();
+    registry->registerModel(MockDataModelCreator(&modelsDestroyed));
+
+    modelsDestroyed = 0;
+
+    FlowScene scene(std::move(registry));
+
+    auto& node = scene.createNode(scene.registry().create("name"));
+
+    // On destruction, if this `node` outlives its MockDataModelCreator,
+    // (if it outlives the DataModelRegistry), then we trigger undefined
+    // behavior through use-after-free. ASAN will catch that.
+  }
+
+  CHECK(modelsDestroyed == 1);
+}