Loki-Astari / ThorsMongo

C++ MongoDB API and BSON/JSON Serialization library
GNU General Public License v3.0
316 stars 72 forks source link

Pure virtual classes don't seem to work (When using CRTP) #89

Closed mjkl-gh closed 1 month ago

mjkl-gh commented 2 months ago

Describe the bug I'm trying to implement a Curiously Recurring Template Pattern that i'd like to serialize. However, I'm running into some issues with Thorserializer. I hope you can help. The implementation looks something like this:

I've got an object that contains a list of tasks, like a to do list. These tasks need to be executed in vastly different ways, so every tasktype inherits from task and is then implemented. Now we'd like to copy (parts) of the todo lists to create new todolists. Somthing like this

main.cpp

#include "ThorSerialize/Serialize.h"
#include "ThorSerialize/Serialize.tpp"
#include "ThorSerialize/Traits.h"
#include "ThorSerialize/JsonThor.h"
#include <string>
#include <sstream>
#include <iostream>

// Base class has a pure virtual function for cloning
class AbstractTask {
public:
    virtual ~AbstractTask () = default;
    virtual std::unique_ptr<AbstractTask> clone() const = 0;

    ThorsAnvil_PolyMorphicSerializer(AbstractTask);
};

// This CRTP class implements clone() for Derived
template <typename Derived>
class Task : public AbstractTask {
public:
    std::unique_ptr<AbstractTask> clone() const override {
        return std::make_unique<Derived>(static_cast<Derived const&>(*this));
    }

    ThorsAnvil_PolyMorphicSerializer(Task);
protected:
   // We make clear Task class needs to be inherited
   Task() = default;
   Task(const Task&) = default;
   Task(Task&&) = default;
};

class Project  {
   public:
    std::vector<std::unique_ptr<AbstractTask>> tasks;    }
};

// Every derived class inherits from CRTP class instead of abstract class

class Cleaning : public Task<Cleaning>{
    std::string nameOfRoom;
    int durationInSeconds;
};

class Sorting : public Task<Sorting>{
    std::vector<int> bucketOfJunk;
};

ThorsAnvil_MakeTrait(AbstractTask);
ThorsAnvil_Template_ExpandTrait(1, AbstractTask, Task, path);
ThorsAnvil_ExpandTrait(Task<Cleaning>, Cleaning, nameOfRoom, durationInSeconds);
ThorsAnvil_ExpandTrait(Task<Sorting>, Sorting, Task, bucketOfJunk);

ThorsAnvil_MakeTrait(Project, tasks);

int main(int argc, char* argv[]) { 
    std::stringstream ss("{"data": "Some Json object"}");

    auto project = std::make_unique<Project>();
    try {
        ss >> jsonImport(*project, thor_config::parser_config);
    } catch (std::exception& e) {
        LOG_ERROR(LOG, "JSON parsing error: " << e.what());
        return nullptr;
    }
 }

However, when trying to compile it, with the ThorsAnvil_PolyMorphicSerializer macro inside AbstractTask, it will try to compile logic for instantiating the purely virtual AbstractTask. Which errors out.

When deleting the macro, it will trip over the same error at jsonImport(*project)

Expected behavior Is this expected behavior? I would expect it not to try and compile logic for instantiating AbstractTasks. The class is only used as an interface for more complex tasks, and such should never be instantiated.

Environment:

Loki-Astari commented 1 month ago

Sorry missed this. Looking now.

Loki-Astari commented 1 month ago

Found an issue with my code: I will try and get a fix in soon but I want to respond with an update first:

Bug in ThorSerializer: It does now work with abstract virtual functions.

    virtual std::unique_ptr<AbstractTask> clone() const  = 0;

Temporary fix needed:

    virtual std::unique_ptr<AbstractTask> clone() const  {return nullptr;}

Bug now fixed.

Bug in your code: Inside the class Task

ThorsAnvil_PolyMorphicSerializer(Task<Derived>); // Notice the <Derived> That is the full name of the type.

I would note that this line is probably not needed and could be removed.

In the class Sorting and Cleaning the members are private. So you need to give access to the serializer. Also these classes need a copy of the ThorsAnvil_PolyMorphicSerializer macro so we can correctly get their type. So should be declared like this:

  class Cleaning : public Task<Cleaning>{
      friend class ThorsAnvil::Serialize::Traits<Cleaning>;
      std::string nameOfRoom;
      int durationInSeconds;
      public:
          ThorsAnvil_PolyMorphicSerializerWithOverride(Cleaning);
  };

  class Sorting : public Task<Sorting>{
      friend class ThorsAnvil::Serialize::Traits<Sorting>;
      std::vector<int> bucketOfJunk;
      public:
          ThorsAnvil_PolyMorphicSerializerWithOverride(Sorting);
  };

Final bug in your code is:

ThorsAnvil_Template_ExpandTrait(1, AbstractTask, Task, path);  // there is no member "path" in AbstractTask

When I add a main to dump a project:

  int main(int argc, char* argv[]) {␣
      using namespace std::string_literals;

      Project     projects;
      projects.tasks.emplace_back(new Sorting{});
      projects.tasks.emplace_back(new Cleaning{});
      std::cout << ThorsAnvil::Serialize::jsonExporter(projects);
}

I get:

    {
        "tasks": [
            {
                "__type": "Sorting",
                "bucketOfJunk": []
            },
            {
                "__type": "Cleaning",
                "nameOfRoom": "",
                "durationInSeconds": 0
            }]
    }
Loki-Astari commented 1 month ago

https://github.com/Loki-Astari/ThorsMongo/issues/91

Loki-Astari commented 1 month ago

https://github.com/Loki-Astari/ThorsSerializer/commit/ac99535cb95c15dd0cd88c3377f9c325812d5495

Loki-Astari commented 1 month ago

Brew build in progress: https://github.com/Homebrew/homebrew-core/pull/182560

Loki-Astari commented 1 month ago

One last note:

            ss >> jsonImport(*project, thor_config::parser_config);

You are using a depricated API. The new version of this is:

            ss >> jsonImporter(*project);
                  //        ^^   The added er.

The main difference is the new one will no throw exceptions (just set the stream to bad) unless you explicitly set the config object to allow exceptions to be propagated).

Loki-Astari commented 1 month ago

After the brew update has finsihed. Or you pull the latest version:

This is the code I run to test:

  #include "ThorSerialize/Traits.h"
  #include "ThorSerialize/JsonThor.h"
  #include "ThorsLogging/ThorsLogging.h"
  #include <string>
  #include <sstream>
  #include <iostream>

  // Base class has a pure virtual function for cloning
  class AbstractTask {
  public:
      virtual ~AbstractTask () = default;
      virtual std::unique_ptr<AbstractTask> clone() const = 0;

       ThorsAnvil_PolyMorphicSerializer(AbstractTask);
  };

  // This CRTP class implements clone() for Derived
  template <typename Derived>
  class Task : public AbstractTask {
  public:
      std::unique_ptr<AbstractTask> clone() const override {
          return std::make_unique<Derived>(static_cast<Derived const&>(*this));
      }

       ThorsAnvil_PolyMorphicSerializerWithOverride(Task<Derived>);
  protected:
     // We make clear Task class needs to be inherited
     Task() = default;
     Task(const Task&) = default;
     Task(Task&&) = default;
  };

  class Project  {
     public:
      std::vector<std::unique_ptr<AbstractTask>> tasks;
  };

  // Every derived class inherits from CRTP class instead of abstract class

  class Cleaning : public Task<Cleaning>{
      friend class ThorsAnvil::Serialize::Traits<Cleaning>;
      std::string nameOfRoom;
      int durationInSeconds;
      public:
          ThorsAnvil_PolyMorphicSerializerWithOverride(Cleaning);
  };

  class Sorting : public Task<Sorting>{
      friend class ThorsAnvil::Serialize::Traits<Sorting>;
      std::vector<int> bucketOfJunk;
      public:
          ThorsAnvil_PolyMorphicSerializerWithOverride(Sorting);
  };

  ThorsAnvil_MakeTrait(AbstractTask);
  ThorsAnvil_Template_ExpandTrait(1, AbstractTask, Task);
  ThorsAnvil_ExpandTrait(Task<Cleaning>, Cleaning, nameOfRoom, durationInSeconds);
  ThorsAnvil_ExpandTrait(Task<Sorting>, Sorting, bucketOfJunk);
  ThorsAnvil_MakeTrait(Project, tasks);

  int main(int argc, char* argv[]) {
      using namespace std::string_literals;

      Project     projects;
      projects.tasks.emplace_back(new Sorting{});
      projects.tasks.emplace_back(new Cleaning{});
      std::cout << ThorsAnvil::Serialize::jsonExporter(projects);

      std::stringstream ss(R"(    {
          "tasks": [
              {
                  "__type": "Sorting",
                  "bucketOfJunk": []
              },
              {
                  "__type": "Cleaning",
                  "nameOfRoom": "",
                  "durationInSeconds": 0
              }]
      })");

      auto project = std::make_unique<Project>();
      try {
          ss >> jsonImporter(*project, ThorsAnvil::Serialize::ParserConfig{});
      } catch (std::exception& e) {
          ThorsLog("main", "exception", "JSON parsing error: ", e.what());
      }
      return 0;
   }
Loki-Astari commented 1 month ago

OK. This is now complete. Closing. Let me know if you want to re-open.

mjkl-gh commented 1 month ago

@Loki-Astari

Many thanks for looking at this, Sorry for any bugs on my end. I wanted to simplify the the example to demonstrate the issue better, but it seems I made some mistakes along the way.

virtual std::unique_ptr<AbstractTask> clone() const {return nullptr;}

Was indeed how we temporarily fixed it. Although our solution was:

virtual std::unique_ptr<AbstractTask> clone() const { throw std::runtime_error("Not_implemented"); }

I'll checkout the latest commit and will report back (and re-open if any issue persists)

Loki-Astari commented 1 month ago

No problem. Would love to know what project you are working on.

mjkl-gh commented 3 weeks ago

Unfortunately, I won't be able to report back on whether the changes fixed the issue or not. I just noticed there's a new dependency on magic_enum since we last looked at this. Unfortunately, we are still on gcc-8, for various reasons, and cannot use that library. We are currently busy transitioning to a newer version, but that will take some time before I can try it out.

Loki-Astari commented 3 weeks ago

magic_enum is not required. If you remove the line that #includes it then it will still work.

BUT: You will then need to use the macro ThorsAnvil_MakeEnum() to handle enum values.

I will add something to the configuration that makes magic_enum optional.

Loki-Astari commented 3 weeks ago

https://github.com/Loki-Astari/ThorsMongo/issues/92