fix for classes that have DoArchive but no inheritance. Some comments

libsrc/core/basearchive.hpp

@@ -150,17 +150,21 @@ namespace ngcore
 
           void* reg_ptr = ptr.get();
           bool neededDowncast = false;
-          if constexpr(has_DoArchive<T>::value)
+          // Downcasting is only possible for our registered classes
-                        {
+          if(typeid(T) != typeid(*ptr))
-                          if(GetArchiveRegister().count(std::string(typeid(*ptr).name())) == 0)
+            {
-                            throw std::runtime_error(std::string("Archive error: Polymorphic type ")
+              if constexpr(has_DoArchive<T>::value)
-                                                     + typeid(*ptr).name()
+                            {
-                                                     + " not registered for archive");
+                              if(GetArchiveRegister().count(std::string(typeid(*ptr).name())) == 0)
-                          else
+                                throw std::runtime_error(std::string("Archive error: Polymorphic type ")
-                            reg_ptr = GetArchiveRegister()[typeid(*ptr).name()].downcaster(typeid(T), ptr.get());
+                                                         + typeid(*ptr).name()
-                          if(reg_ptr != (void*) ptr.get())
+                                                         + " not registered for archive");
-                            neededDowncast = true;
+                              reg_ptr = GetArchiveRegister()[typeid(*ptr).name()].downcaster(typeid(T), ptr.get());
-                        }
+                              // if there was a true downcast we have to store more information
+                              if(reg_ptr != (void*) ptr.get())
+                                neededDowncast = true;
+                            }
+            }
           auto pos = shared_ptr2nr.find(reg_ptr);
           // if not found store -1 and the pointer
           if(pos == shared_ptr2nr.end())
@@ -168,6 +172,7 @@ namespace ngcore
               auto p = ptr.get();
               (*this) << -1;
               (*this) & neededDowncast & p;
+              // if we did downcast we store the true type as well
               if(neededDowncast)
                 (*this) << std::string(typeid(*ptr).name());
               shared_ptr2nr[reg_ptr] = shared_ptr_count++;
@@ -183,22 +188,27 @@ namespace ngcore
         {
           int nr;
           (*this) & nr;
+          // -2 restores a nullptr
           if(nr == -2)
             {
               ptr = nullptr;
               return *this;
             }
+          // -1 restores a new shared ptr by restoring the inner pointer and creating a shared_ptr to it
           else if (nr == -1)
             {
               T* p;
               bool neededDowncast;
               (*this) & neededDowncast & p;
               ptr = std::shared_ptr<T>(p);
+              // if we did downcast we need to store a shared_ptr<void> to the true object
               if(neededDowncast)
                 {
                   std::string name;
                   (*this) & name;
                   auto info = GetArchiveRegister()[name];
+                  // for this we use an aliasing constructor to create a shared pointer sharing lifetime
+                  // with our shared ptr, but pointing to the true object
                   nr2shared_ptr.push_back(std::shared_ptr<void>(std::static_pointer_cast<void>(ptr),
                                                                 info.downcaster(typeid(T),
                                                                                 ptr.get())));
@@ -213,11 +223,15 @@ namespace ngcore
               (*this) & neededDowncast;
               if(neededDowncast)
                 {
+                  // if there was a downcast we can expect the class to be registered (since archiving
+                  // wouldn't have worked else)
                   if constexpr(has_DoArchive<T>::value)
                                 {
                                   std::string name;
                                   (*this) & name;
                                   auto info = GetArchiveRegister()[name];
+                                  // same trick as above, create a shared ptr sharing lifetime with
+                                  // the shared_ptr<void> in the register, but pointing to our object
                                   ptr = std::static_pointer_cast<T>(std::shared_ptr<void>(other,
                                                                                           info.upcaster(typeid(T),
                                                                                                other.get())));
@@ -240,21 +254,20 @@ namespace ngcore
         {
           // if the pointer is null store -2
           if (!p)
-            {
+            return (*this) << -2;
-              int m2 = -2;
-              (*this) & m2;
-              return *this;
-            }
           void* reg_ptr = (void*)p;
-          if constexpr(has_DoArchive<T>::value)
+          if(typeid(T) != typeid(*p))
-                        {
+            {
-                          if(GetArchiveRegister().count(std::string(typeid(*p).name())) == 0)
+              if constexpr(has_DoArchive<T>::value)
-                            throw std::runtime_error(std::string("Archive error: Polimorphic type ")
+                            {
-                                                     + typeid(*p).name()
+                              if(GetArchiveRegister().count(std::string(typeid(*p).name())) == 0)
-                                                     + " not registered for archive");
+                                throw std::runtime_error(std::string("Archive error: Polymorphic type ")
-                          else
+                                                         + typeid(*p).name()
-                            reg_ptr = GetArchiveRegister()[typeid(*p).name()].downcaster(typeid(T), p);
+                                                         + " not registered for archive");
-                        }
+                              else
+                                reg_ptr = GetArchiveRegister()[typeid(*p).name()].downcaster(typeid(T), p);
+                            }
+            }
           auto pos = ptr2nr.find(reg_ptr);
           // if the pointer is not found in the map create a new entry
           if (pos == ptr2nr.end())
@@ -270,11 +283,14 @@ namespace ngcore
                                            typeid(*p).name() + " does not provide a default constructor!");
               else
                 {
-                  // We want this special behaviour only for our classes that implement DoArchive
+                  // if a pointer to a base class is archived, the class hierarchy must be registered
+                  // to avoid compile time issues we allow this behaviour only for "our" classes that
+                  // implement a void DoArchive(Archive&) member function
+                  // To recreate the object we need to store the true type of it
                   if constexpr(has_DoArchive<T>::value)
                                 {
                                   if(GetArchiveRegister().count(std::string(typeid(*p).name())) == 0)
-                                    throw std::runtime_error(std::string("Archive error: Polimorphic type ")
+                                    throw std::runtime_error(std::string("Archive error: Polymorphic type ")
                                                              + typeid(*p).name()
                                                              + " not registered for archive");
                                   else
@@ -289,18 +305,18 @@ namespace ngcore
           else
             {
               (*this) & pos->second;
-              (*this) << std::string(typeid(*p).name());
+              bool downcasted = !(reg_ptr == (void*) p);
+              // store if the class has been downcasted and the name
+              (*this) << downcasted << std::string(typeid(*p).name());
             }
         }
       else
         {
           int nr;
           (*this) & nr;
-          if (nr == -2)
+          if (nr == -2) // restore a nullptr
-            {
               p = nullptr;
-            }
+          else if (nr == -1) // create a new pointer of standard type (no virtual or multiple inheritance,...)
-          else if (nr == -1)
             {
               if constexpr (std::is_constructible<T>::value)
                              {
@@ -312,15 +328,19 @@ namespace ngcore
                 throw std::runtime_error("Class isn't registered properly");
 
             }
-          else if(nr == -3)
+          else if(nr == -3) // restore one of our registered classes that can have multiple inheritance,...
             {
-              // We want this special behaviour only for our classes that implement DoArchive
+              // As stated above, we want this special behaviour only for our classes that implement DoArchive
               if constexpr(has_DoArchive<T>::value)
                             {
                               std::string name;
                               (*this) & name;
                               auto info = GetArchiveRegister()[name];
+                              // the creator creates a new object of type name, and returns a void* pointing
+                              // to T (which may have an offset)
                               p = (T*) info.creator(typeid(T));
+                              // we store the downcasted pointer (to be able to find it again from
+                              // another class in a multiple inheritance tree)
                               nr2ptr.push_back(info.downcaster(typeid(T),p));
                               (*this) & *p;
                             }
@@ -329,13 +349,18 @@ namespace ngcore
             }
           else
             {
+              bool downcasted;
               std::string name;
-              (*this) & name;
+              (*this) & downcasted & name;
-              if constexpr(has_DoArchive<T>::value)
+              if(downcasted)
-                            {
+                {
-                              auto info = GetArchiveRegister()[name];
+                  // if the class has been downcasted we can assume it is in the register
-                              p = (T*) info.upcaster(typeid(T), nr2ptr[nr]);
+                  if constexpr(has_DoArchive<T>::value)
-                            }
+                                {
+                                  auto info = GetArchiveRegister()[name];
+                                  p = (T*) info.upcaster(typeid(T), nr2ptr[nr]);
+                                }
+                }
               else
                 p = (T*) nr2ptr[nr];
             }

tests/catch/archive.cpp

@@ -6,10 +6,11 @@ using namespace std;
 
 class CommonBase
 {
-  public:
+public:
+  int a;
   virtual ~CommonBase() {}
 
-  virtual void DoArchive(Archive& archive) { }
+  virtual void DoArchive(Archive& archive) { archive & a; }
 };
 
 class SharedPtrHolder : virtual public CommonBase
@@ -19,7 +20,11 @@ public:
   virtual ~SharedPtrHolder()
   { }
 
-  virtual void DoArchive(Archive& archive) {  archive & names; }
+  virtual void DoArchive(Archive& archive)
+  {
+    CommonBase::DoArchive(archive);
+    archive & names;
+  }
 };
 
 class PtrHolder : virtual public CommonBase
@@ -28,7 +33,11 @@ public:
   vector<int*> numbers;
   virtual ~PtrHolder() {}
 
-  virtual void DoArchive(Archive& archive) {  archive & numbers; }
+  virtual void DoArchive(Archive& archive)
+  {
+    CommonBase::DoArchive(archive);
+    archive & numbers;
+  }
 };
 
 class SharedPtrAndPtrHolder : public SharedPtrHolder, public PtrHolder
@@ -42,6 +51,18 @@ public:
   }
 };
 
+// Classes without virt. or multiple inheritance do not need to be registered
+class SimpleClass : public CommonBase
+{
+public:
+  double d;
+  virtual void DoArchive(Archive& ar)
+  {
+    CommonBase::DoArchive(ar);
+    ar & d;
+  }
+};
+
 class NotRegisteredForArchive : public SharedPtrAndPtrHolder {};
 
 class OneMoreDerivedClass : public SharedPtrAndPtrHolder {};
@@ -52,6 +73,19 @@ static RegisterClassForArchive<PtrHolder, CommonBase> regp;
 static RegisterClassForArchive<SharedPtrAndPtrHolder, SharedPtrHolder, PtrHolder> regspp;
 static RegisterClassForArchive<OneMoreDerivedClass, SharedPtrAndPtrHolder> regom;
 
+void testNullPtr(Archive& in, Archive& out)
+{
+  SharedPtrHolder* p = nullptr;
+  shared_ptr<string> sp = nullptr;
+  out & p & sp;
+  out.FlushBuffer();
+  SharedPtrHolder* pin;
+  shared_ptr<string> spin;
+  in & pin & spin;
+  CHECK(pin == nullptr);
+  CHECK(spin == nullptr);
+}
+
 void testSharedPointer(Archive& in, Archive& out)
 {
   SECTION("Same shared ptr")
@@ -92,6 +126,7 @@ void testMultipleInheritance(Archive& in, Archive& out)
 {
   PtrHolder* p = new OneMoreDerivedClass;
   p->numbers.push_back(new int(2));
+  p->a = 5;
   auto p2 = dynamic_cast<SharedPtrHolder*>(p);
   p2->names.push_back(make_shared<string>("test"));
   auto sp1 = shared_ptr<PtrHolder>(p);
@@ -103,6 +138,8 @@ void testMultipleInheritance(Archive& in, Archive& out)
       CHECK(*pin2->names[0] == "test");
       CHECK(*pin->numbers[0] == 2);
       CHECK(dynamic_cast<SharedPtrAndPtrHolder*>(pin) == dynamic_cast<SharedPtrAndPtrHolder*>(pin2));
+      CHECK(pin->a == pin2->a);
+      CHECK(pin->a == 5);
       REQUIRE(dynamic_cast<SharedPtrAndPtrHolder*>(pin2) != nullptr);
       CHECK(*dynamic_cast<SharedPtrAndPtrHolder*>(pin2)->numbers[0] == 2);
       CHECK(*pin->numbers[0] == *dynamic_cast<SharedPtrAndPtrHolder*>(pin2)->numbers[0]);
@@ -136,6 +173,31 @@ void testMultipleInheritance(Archive& in, Archive& out)
       in & bin & pin;
       checkPtr(pin, dynamic_cast<SharedPtrHolder*>(bin));
     }
+  SECTION("Simple class without register")
+    {
+      auto a = new SimpleClass;
+      a->a = 5;
+      a->d = 2.3;
+      SECTION("check pointer")
+        {
+          out & a;
+          out.FlushBuffer();
+          SimpleClass* ain;
+          in & ain;
+          CHECK(ain->a == 5);
+          CHECK(ain->d == 2.3);
+        }
+      SECTION("check shared pointer")
+        {
+          auto spa = shared_ptr<SimpleClass>(a);
+          out & spa;
+          out.FlushBuffer();
+          shared_ptr<SimpleClass> spain;
+          in & spain;
+          CHECK(spain->a == 5);
+          CHECK(spain->d == 2.3);
+        }
+    }
 }
 
 void testArchive(Archive& in, Archive& out)
@@ -157,6 +219,10 @@ void testArchive(Archive& in, Archive& out)
       SharedPtrAndPtrHolder* p = new NotRegisteredForArchive;
       REQUIRE_THROWS(out & p, Catch::Contains("not registered for archive"));
     }
+  SECTION("nullptr")
+    {
+      testNullPtr(in, out);
+    }
 }
 
 TEST_CASE("BinaryArchive")