Fix default constructors/operators of Container and BaseRecordComponent

These derive virtually from Attributable, and this avoids that pitfalls propagate to user code.
franzpoeschel · Apr 14, 2023 · 969efba · 969efba
1 parent 4b2c420
commit 969efba
Show file tree

Hide file tree

Showing 2 changed files with 90 additions and 0 deletions.
diff --git a/include/openPMD/backend/BaseRecordComponent.hpp b/include/openPMD/backend/BaseRecordComponent.hpp
@@ -67,6 +67,51 @@ class BaseRecordComponent : virtual public Attributable
     friend class Container;
 
 public:
+    /*
+     * Need to define these manually due to the virtual inheritance from
+     * Attributable.
+     * Otherwise, they would only run from the most derived class
+     * if explicitly called.
+     * If not defining these, a user could destroy copy/move constructors/
+     * assignment operators by deriving from any class that has a virtual
+     * Attributable somewhere.
+     * Care must be taken in move constructors/assignment operators to not move
+     * multiple times (which could happen in diamond inheritance situations).
+     */
+
+    BaseRecordComponent(BaseRecordComponent const &other)
+        : Attributable(NoInit())
+    {
+        m_attri = other.m_attri;
+        m_baseRecordComponentData = other.m_baseRecordComponentData;
+    }
+
+    BaseRecordComponent(BaseRecordComponent &&other) : Attributable(NoInit())
+    {
+        if (other.m_attri)
+        {
+            m_attri = std::move(other.m_attri);
+        }
+        m_baseRecordComponentData = std::move(other.m_baseRecordComponentData);
+    }
+
+    BaseRecordComponent &operator=(BaseRecordComponent const &other)
+    {
+        m_attri = other.m_attri;
+        m_baseRecordComponentData = other.m_baseRecordComponentData;
+        return *this;
+    }
+
+    BaseRecordComponent &operator=(BaseRecordComponent &&other)
+    {
+        if (other.m_attri)
+        {
+            m_attri = std::move(other.m_attri);
+        }
+        m_baseRecordComponentData = std::move(other.m_baseRecordComponentData);
+        return *this;
+    }
+
     double unitSI() const;
 
     BaseRecordComponent &resetDatatype(Datatype);

diff --git a/include/openPMD/backend/Container.hpp b/include/openPMD/backend/Container.hpp
@@ -457,6 +457,51 @@ OPENPMD_protected
 
     Container(NoInit) : Attributable(NoInit())
     {}
+
+public:
+    /*
+     * Need to define these manually due to the virtual inheritance from
+     * Attributable.
+     * Otherwise, they would only run from the most derived class
+     * if explicitly called.
+     * If not defining these, a user could destroy copy/move constructors/
+     * assignment operators by deriving from any class that has a virtual
+     * Attributable somewhere.
+     * Care must be taken in move constructors/assignment operators to not move
+     * multiple times (which could happen in diamond inheritance situations).
+     */
+
+    Container(Container const &other) : Attributable(NoInit())
+    {
+        m_attri = other.m_attri;
+        m_containerData = other.m_containerData;
+    }
+
+    Container(Container &&other) : Attributable(NoInit())
+    {
+        if (other.m_attri)
+        {
+            m_attri = std::move(other.m_attri);
+        }
+        m_containerData = std::move(other.m_containerData);
+    }
+
+    Container &operator=(Container const &other)
+    {
+        m_attri = other.m_attri;
+        m_containerData = other.m_containerData;
+        return *this;
+    }
+
+    Container &operator=(Container &&other)
+    {
+        if (other.m_attri)
+        {
+            m_attri = std::move(other.m_attri);
+        }
+        m_containerData = std::move(other.m_containerData);
+        return *this;
+    }
 };
 
 namespace internal